load event handling for iframes with no src may not be web compatible

[bzbarsky]

Consider this testcase:

var iframe = document.createElement('iframe');
document.body.appendChild(iframe);
var fn = () => {
  iframe.remove();
  console.log('error');
};
iframe.addEventListener('load', fn);
iframe.src = "https://www.seattlesymphony.org/~/media/files/notes/schubert-sonata-21-b-flat.pdf";

(where you can use any URL that has Content-Disposition: attachment for the src). Per spec what should happen here is this:

1. During the appendChild call https://html.spec.whatwg.org/multipage/iframe-embed-object.html#process-the-iframe-attributes runs and does the "Queue a task to run the iframe load event steps." bit.
2. The event listener is added.
3. The load in the iframe starts.
4. The queued task from step 1 runs, fires the load event, the iframe is removed and "error" is logged.

What happens in browsers instead is:

• Chrome and Safari: The load event fires synchronously under the appendChild call, as far as I can tell, so that the listener is added after the event has fired and hence the frame is never removed and "error" is never logged. The navigation does not fire a load event, because it has Content-Disposition: attachment and hence is not loaded in the iframe.
• Firefox: The insertion starts an async about:blank load which would fire a load event async when it completes. The start of a new navigation cancels that load, so there is no load event fired at all. The frame is never removed and "error" is never logged. The navigation to the PDF does not fire a load event, because it has Content-Disposition: attachment and hence is not loaded in the iframe.

I don't have a good proposal for what the spec should say here... Currently the spec-compliant way for a page to do what the above script is trying to do is something like:

var iframe = document.createElement('iframe');
iframe.addEventListener("load", () => {
    var fn = () => {
      iframe.remove();
      console.log('error');
    };
    iframe.addEventListener('load', fn);
    iframe.src = "https://www.seattlesymphony.org/~/media/files/notes/schubert-sonata-21-b-flat.pdf";
  }, { once: true });
document.body.appendChild(iframe);

which is rather annoying from an authoring perspective and hard for authors to think of.

That said, I expect that parser-triggered insertions likely do need the async load event thing in some way, though this testcase:

  <iframe onload="console.log('sync-loaded')"></iframe>
  <script>
    document.querySelector("iframe").onload = () => { console.log('async-loaded'); }
  </script>

shows "sync-loaded" in Chrome and Safari (and "async-loaded" in Firefox; per spec behavior could be either way depending on whether there's a packet boundary between the <iframe> and the <script>, afaict).

[bzbarsky]

Totally forgot to cc people: @annevk @foolip @domenic @TimothyGu

[foolip]

I haven't verified in debug build, but I suspect https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/local_dom_window.cc?l=1383&rcl=ef5184e1d38e632e1b12588586c473fc0e02237b is the place in Chromium that fires that sync load event, and suspect that it's because of the initial "about:blank" page being loaded synchronously when inserting the iframe.

The preceding comment is suspicious:

  // For load events, send a separate load event to the enclosing frame only.
  // This is a DOM extension and is independent of bubbling/capturing rules of
  // the DOM.

It's unsurprising that this code predates any spec on this, but now the spec is pretty clear in "When an iframe element element is inserted into a document whose browsing context is non-null, the user agent must run these steps" that the browsing context is created sync but the event is fired in a queued task.

@bzbarsky it sounds like in addition to the synchronicity of the event dispatch you also think some spec change is needed around Content-Disposition: attachment? Where did you come across the original snippet of code, and was that event handler really there to detect an error, as opposed to just copy-pasted from some other piece of code where the load event was fired? I'm guessing this was a site compat issue where Firefox was broken by that event handler running, and it did some things that weren't intended?

[foolip]

Getting further through my inbox, I suspect https://bugs.chromium.org/p/chromium/issues/detail?id=878491#c4 may be related to this, although that also involves document.close().

[annevk]

I'm not sure I understand either. There's a problem with the initial load event, but it also sounds like Chrome/Safari do fire a load for "downloads" and Firefox does not. I'm not sure the specification does so I'm not sure if the "spec-compliant way" is indeed working in theory or would also end up removing the iframe element too early.

[bzbarsky]

Where did you come across the original snippet of code

The original snippet of code that triggered my investigation into this was https://bugzilla.mozilla.org/show_bug.cgi?id=1575799#c16 and does indeed involve load event bits for document.close but is also affected by this issue: if browsers were following the spec here, there would be a load event fired for the initial frame insertion after the document.close() call no matter what else is going on, right?

I suggested a modification to the page in question in https://bugzilla.mozilla.org/show_bug.cgi?id=1575799#c18 and then realized that that modification would stop working in Firefox once Firefox started following the spec for initial about:blank creation better. The actual in-the-wild code involved is more like https://bugzilla.mozilla.org/show_bug.cgi?id=1575799#c21 and they are planning to change it to use DOM operations instead of document.open/close, but it will break again in any browser that starts following the spec for the load event on iframe insertion, since that load event will fire async after they have added the listener.

you also think some spec change is needed around Content-Disposition: attachment

No, I don't think so...

but it also sounds like Chrome/Safari do fire a load for "downloads" and Firefox does not

None of them fire a load event for downloads, and the specification does not either. The "spec-compliant way" works in every browser and would continue working if they all aligned with the spec in all the relevant ways; it's just complicated and non-intuitive.

[foolip]

If part of the problem is that making the load event fire async will sometimes fire it where it's currently assumed to not fire, is there any set of conditions that the queued task could check and decide to not fire the event? Media elements do something like this to avoid firing events originating from one invocation of the media load algorithm after it has restarted for another resource. Doing this for browsing contexts is probably more complicated, but would it fix any of the problem?

[annevk]

Firing load synchronously during appendChild() might be okay and would make the parser case more predictable (instead of depending on packet boundaries). It's effectively a mutation event though and we haven't defined those (yet) so putting it in the HTML Standard might not be as straightforward as one would hope (at least if you want to make sure it's all well defined).

If you're okay with experimenting with it in Firefox I wouldn't mind.

[bzbarsky]

Well, I don't quite understand the semantics Chrome and Safari (e.g. is this limited to no-src cases, or also including src = about:blank, or something else where the browser knows synchronously that it's done, e.g. a srcdoc with no subresources). That makes it a bit hard to experiment...

[annevk]

I see, so testing blob: URLs (let's hope not), data: URLs (same), srcdoc, and explicit about:blank is needed to move this forward. I can work on that. (Chrome and Safari both fail html/semantics/embedded-content/the-iframe-element/iframe-load-event.html though the reason seems to be about IDL in part.)

[annevk]

@bzbarsky web-platform-tests/wpt#19693 has some tests, further suggestions welcome. They're currently aligned with Chrome/Safari in that no src and src=about:blank fire load during append (did not test relative to mutation events) and everything else fires load "asynchronously".

[bzbarsky]

How do things behave when there are multiple iframe elements being inserted, whether via a single append operation on an ancestor, via document fragment insertion, via innerHTML, etc? What does ordering for the load events look like and how does it order with the rest of the insertion process?

[bzbarsky]

is there any set of conditions that the queued task could check and decide to not fire the event?

Possibly, yes. In particular, whether a new navigation has started. We could also track the task that's queued and "cancel" it when a navigation starts.

That said, if we can spec the "just fire it sync on insertion" behavior, great.

[annevk]

I added a simple test for multiple iframe elements to the above PR and the result is similar to the script element (which has a similar issue and remains unsolved, can't be bothered to cross-link it atm). Chrome inserts all, then executes one-by-one (so the first load event can observe the other elements), Safari inserts-and-executes one-by-one.

Given that there's precedent with script that I forgot about I'm a little less worried about tentatively concluding no src/src=about:blank should become "synchronous" in the HTML Standard.

At this point I'd be happy to help someone make these changes, but I'm probably not gonna do it all myself until the priority rises.

[bzbarsky]

@foolip It looks like a bunch of WPTs were written assuming the non-spec Chrome behavior here, which made them completely break in Firefox. See https://bugzilla.mozilla.org/show_bug.cgi?id=1598674

[foolip]

Thanks for pointing that out, @bzbarsky!

@stephenmcgruer can you dig into what happened here, how were the tests added to wpt and what tooling would we need in place to notice this sort of problem before it's merged?

[annevk]

I realized that in Chrome it's possible for an iframe element to be connected and not have a nested browsing context (since that is created when load is dispatched, which is post-insertion; observable through inserting multiple iframe elements or with a script element), but this is a condition the HTML Standard already accounts for in its various accessors.

web-platform-tests/wpt#19693 has more tests/assertions for those now. It's also a little weird that sometimes a nested browsing context is created with an event and sometimes without.

[stephenmcgruer]

@foolip Apologies for the delay. I'm not immediately clear on reading this thread why we would have expected these tests to be caught? Current tooling for Chromium-exported tests doesn't require a test to pass on any given browser, it just requires the results to be consistent.

We do have the (slightly strangely named) Export Notifier project which will hopefully (re)launch Q1. This would let Chromium CL authors know that a test is newly failing in some browser (which should include new tests, I imagine), but we would still not want to automatically block those cases. (Many legitimate tests fail on some browser).

Overall, having invalid tests sneak in is the price of a shared test suite, and I consider it worth the effort to find and fix such tests (since they often expose interop issues between browsers!). Hopefully that effort is spread reasonably evenly over browsers (I know animations team have had to squash some tests that were invalid and failing in Chrome), but if it's not then we should consider what else we could do to resolve that.

[bzbarsky]

Note that there is also a test at https://github.com/web-platform-tests/wpt/blob/master/html/semantics/embedded-content/the-iframe-element/iframe-nosrc.html that tests this behavior and has a currently-spec-unsupported assertion about "load event of iframe should not be fired after processing the element".

[foolip]

@stephenmcgruer the title of https://bugzilla.mozilla.org/show_bug.cgi?id=1598674 says "Hundreds of timeouts" so maybe just summarizing the results (e.g. "Firefox: 103 new tests, 103 timeout") would bring attention to it where that's surprising.

I'm struggling to find a single wpt PR that added the tests, web-platform-tests/wpt#4292 is the oldest but web-platform-tests/wpt#4292 (comment) actually looks good.

Huge numbers of timeouts will make the tests run slower and can often be turned into a fast fail, but it's not a case that I think we can require any action for when the tests are new as here. Regressing to timeout is a different matter and probably worth flagging.