Fix iframe.onload being fired twice on about:blank. #16091

gpoesia · 2017-03-23T01:12:19Z

Supress the load event in the script thread when the iframe doesn't have a src attribute.
In that case, a load event is already fired when binding the iframe to the tree.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15727 (github issue number if applicable).
There are tests for these changes OR

This change is

highfive · 2017-03-23T01:12:24Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/script_thread.rs
@KiChjang: components/script/script_thread.rs

highfive · 2017-03-23T01:12:25Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

cbrewster

Looks good, although I did find another edge case ☹️

cbrewster · 2017-03-23T03:34:01Z

tests/wpt/web-platform-tests/html/webappapis/scripting/events/iframe-onload.html

+      }
+    });
+  }, "IFrameSingleOnLoad");
+  document.getElementById('blank_iframe').contentWindow.location.reload();


I'm not sure if this causes the same behavior as doing an inline onload on the iframe. I wonder if we could just create and add an iframe via javascript. (I would test this first on the master branch and see if that yields the onload event being fired twice).

I had tested it on master and it did trigger two load events. However, I agree it's better to add a new iframe, since it's simpler and doesn't raise doubts. Will do!

cbrewster · 2017-03-23T03:34:01Z

tests/wpt/web-platform-tests/html/webappapis/scripting/events/iframe-onload.html

+  function iframeOnLoad(t) {
+    iframe_loads++;
+
+    setTimeout(function() {


Unfortunately, I am not entirely sure if there is a better option than doing a setTimeout.

cbrewster · 2017-03-23T03:34:01Z

components/script/script_thread.rs

+            Some(iframe) => {
+                // If the iframe has no "src" attribute, a load event will have already been fired
+                // when binding the iframe to the tree.
+                if iframe.upcast::<Element>().has_attribute(&local_name!("src")) {


I have been thinking about this and testing a bit more... I don't know if this will be a sufficient solution. Consider the following case:

You have the iframe with no src

The load event steps for the initial about:blank are fired async as per the spec

In a script you set the src of the iframe to some URL

The load event steps are now processed and a load event is fired for the initial about:blank

A possible fix is to cancel the load event steps runnable when the src is set.
I have been working on about:blank fixes (#14764) and here is a commit that does this 46220d4
I would be ok if you wanted to implement something like that here since I don't know how soon I will have enough time to finish up the rest of that PR

Hmm, right, now I see that case. I couldn't reproduce it, though (I tried this:

<html> <body> <iframe id="iframe" onload="alert('onload');"></iframe> <script type="text/javascript"> document.getElementById("iframe").src = "a.html"; </script> </body> </html>

)

Does the spec say anything on what should happen to the first load event in the case of changing the src? I looked at your commit, but if I got it correctly it will only prevent the load steps from being loaded as long as the IFrameLoadEventSteps runnable checks the cancel flag after it is set, right? I'm imagining a case in which the program is exactly before this.iframe_load_event_steps(self.pipeline_id);, but after the if condition has been evaluated, and at this point the src changes and the other thread tries to cancel the event. The check makes it even harder for both events to run, but does it really guarantee it?
If you think it helps I can merge that commit into this PR. What do you think?

So my solution may not be the best way, the spec basically says to delay the load event if a new navigation is triggered: https://html.spec.whatwg.org/multipage/embedded-content.html#iframe-load-event-steps (See the first NOTE).

The code is not running multithreaded here, we just have asynchronous tasks. So when we queue a task to do the iframe load event steps, this will allow other things to potentially be processed first (i.e. src changing before the task is handled). We currently have a LoadBlocker for Documents that handle this. Maybe we should do something similar with iframes.

nox · 2017-03-24T10:49:33Z

r? @cbrewster

gpoesia · 2017-03-25T22:39:40Z

@cbrewster Oh, I see, thanks for the clarifications and patience! For some reason I thought we had multiple threads involved. I've merged your solution into this commit then, and also modified the test to create the iframe in JavaScript instead. Would you take another look?

cbrewster

Looks like there were some unintended changes in the MANIFEST

cbrewster · 2017-03-26T18:41:25Z

tests/wpt/metadata/MANIFEST.json

@@ -54200,6 +54200,16 @@
     {}
    ]
   ],
+   "html/webappapis/scripting/events/iframe-onload-frame.html": [


cbrewster · 2017-03-26T18:41:25Z

tests/wpt/metadata/MANIFEST.json

+     {}
+    ]
+   ],
+   "html/webappapis/scripting/events/iframe-onload.html2": [


cbrewster · 2017-03-26T18:41:25Z

tests/wpt/metadata/MANIFEST.json

@@ -179815,6 +179831,18 @@
   "491fd73a4ec8812cb8dc1ee01e34a7ff2a07ffb3",
   "testharness"
  ],
+  "html/webappapis/scripting/events/iframe-onload-frame.html": [


cbrewster · 2017-03-26T18:41:25Z

tests/wpt/metadata/MANIFEST.json

+   "d5e1649c88102f27da5fe1ac16715cfee7f70f84",
+   "testharness"
+  ],
+  "html/webappapis/scripting/events/iframe-onload.html2": [


cbrewster · 2017-03-26T18:41:25Z