Script iframe intial load replaces about blank #18587

asajeffrey · 2017-09-21T00:31:42Z

When navigating from the initial about:blank, do it with replacement enabled.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #18564
These changes do not require tests because the existing tests do the job

This change is


        The navigation from the initial about:blank should be a relacement

highfive · 2017-09-21T00:31:46Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/htmliframeelement.rs
@KiChjang: components/script/dom/htmliframeelement.rs

asajeffrey · 2017-09-21T00:31:58Z

r? @cbrewster

asajeffrey · 2017-09-21T00:32:41Z

I did a first pass of updating the test expectations, but it's not perfect.

asajeffrey · 2017-09-21T00:35:47Z

@bors-servo try

bors-servo · 2017-09-21T00:35:52Z

⌛ Trying commit e527d7f with merge a4894e9...


        Auto merge of #18587 - asajeffrey:script-iframe-intial-load-replaces-…

…about-blank, r=<try> Script iframe intial load replaces about blank  When navigating from the initial about:blank, do it with replacement enabled. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18564 - [X] These changes do not require tests because the existing tests do the job    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18587)

bors-servo · 2017-09-21T01:22:46Z

💔 Test failed - linux-rel-wpt

asajeffrey · 2017-09-21T15:29:49Z

Only five failures!

asajeffrey · 2017-09-21T15:57:41Z

Hmm... fun with document loading:

$ RUST_LOG=script::document_loader ./mach test-wpt --debug-build --no-pause /fetch/api/request/multi-globals/url-parsing.html
...
 0:05.12 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) Full command: /home/ajeffrey/github/asajeffrey/servo/target/debug/servo -w --hard-fail -u Servo/wptrunner -Z replace-surrogates -z http://web-platform.test:8000/fetch/api/request/multi-globals/url-parsing.html --user-stylesheet /home/ajeffrey/github/asajeffrey/servo/resources/ahem.css --certificate-path /tmp/tmp7uTOtg/cacert.pem
(pid:20715) "VMware, Inc."
 0:05.12 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "Gallium 0.4 on softpipe"
 0:05.12 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "3.3 (Core Profile) Mesa 17.2.0-devel"
 0:05.27 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Initial blocking load Some("http://web-platform.test:8000/fetch/api/request/multi-globals/url-parsing.html")."
 0:05.27 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Adding blocking load Script("http://web-platform.test:8000/resources/testharness.js") (1)."
 0:05.35 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Adding blocking load Script("http://web-platform.test:8000/resources/testharnessreport.js") (2)."
 0:05.35 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load Script("http://web-platform.test:8000/resources/testharness.js") (3)."
 0:05.36 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Adding blocking load Subframe("about:blank") (2)."
 0:05.36 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Initial blocking load Some("about:blank")."
 0:05.36 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load PageSource("about:blank") (1)."
 0:05.36 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load Subframe("about:blank") (3)."
 0:05.36 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Adding blocking load Subframe("http://web-platform.test:8000/fetch/api/request/multi-globals/incumbent/incumbent.html") (2)."
 0:05.37 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load PageSource("http://web-platform.test:8000/fetch/api/request/multi-globals/url-parsing.html") (3)."
 0:06.11 LOG: Thread-TestrunnerManager-2 WARNING u'runner_teardown' ()
 0:06.11 LOG: Thread-TestrunnerManager-2 INFO No more tests
 0:06.11 LOG: Thread-TestrunnerManager-3 WARNING u'runner_teardown' ()
 0:06.11 LOG: Thread-TestrunnerManager-3 INFO No more tests
 0:06.37 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load Script("http://web-platform.test:8000/resources/testharnessreport.js") (2)."
 0:07.06 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Initial blocking load Some("http://web-platform.test:8000/fetch/api/request/multi-globals/incumbent/incumbent.html")."
 0:07.07 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Adding blocking load Subframe("about:blank") (1)."
 0:07.07 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Initial blocking load Some("about:blank")."
 0:07.07 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load PageSource("about:blank") (1)."
 0:07.07 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load Subframe("about:blank") (2)."
 0:07.07 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Adding blocking load Subframe("http://web-platform.test:8000/fetch/api/request/multi-globals/current/current.html") (1)."
 0:07.08 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load PageSource("http://web-platform.test:8000/fetch/api/request/multi-globals/incumbent/incumbent.html") (2)."
 0:08.44 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load Subframe("http://web-platform.test:8000/fetch/api/request/multi-globals/incumbent/incumbent.html") (1)."
 0:09.13 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Initial blocking load Some("http://web-platform.test:8000/fetch/api/request/multi-globals/current/current.html")."
 0:09.14 PROCESS_OUTPUT: Thread-TestrunnerManager-1 (pid:20715) "DEBUG:script::document_loader: Removing blocking load PageSource("http://web-platform.test:8000/fetch/api/request/multi-globals/current/current.html") (1)."
 0:09.20 TEST_END: Thread-TestrunnerManager-1 Harness OK. Subtests passed 0/1. Unexpected 1
should parse the URL relative to the current settings object
------------------------------------------------------------
Expected PASS, got FAIL
promise_test: Unhandled rejection with value: object "TypeError: Url could not be parsed"
 0:09.20 LOG: Thread-TestrunnerManager-1 WARNING u'runner_teardown' ()
 0:09.20 LOG: Thread-TestrunnerManager-1 INFO No more tests
 0:10.21 LOG: Thread-TestrunnerManager-1 INFO Test runner process shut down
 0:10.21 LOG: Thread-TestrunnerManager-1 WARNING More tests found, but runner process died, restarting
 0:10.21 LOG: Thread-TestrunnerManager-1 INFO No more tests
 0:10.21 LOG: MainThread INFO Got 1 unexpected results
 0:10.21 SUITE_END: MainThread 
Summary
=======

Ran 2 tests (1 parents, 1 subtests)
Expected results: 1
Unexpected results: 1 (FAIL: 1)

Unexpected Results
==================

/fetch/api/request/multi-globals/url-parsing.html
-------------------------------------------------
FAIL should parse the URL relative to the current settings object
 0:10.25 LOG: MainThread INFO Closing logging queue
 0:10.25 LOG: MainThread INFO queue closed

The document loader for incumbent.html completes before the loader for current.html starts. Not good.


        Don't remove the load blocker when traversing an iframe.

asajeffrey · 2017-09-21T21:13:34Z

IRC conversation with @cbrewster: http://logs.glob.uno/?c=mozilla%23servo&s=21+Sep+2017&e=21+Sep+2017#c756805

TL;DR:

servo/components/script/dom/htmliframeelement.rs

Lines 307 to 312 in 581f0bf

    
           // Only terminate the load blocker if the pipeline id was updated due to a traversal. 
        
           // The load blocker will be terminated for a navigation in iframe_load_event_steps. 
        
           if reason == UpdatePipelineIdReason::Traversal { 
        
               let mut blocker = self.load_blocker.borrow_mut(); 
        
               LoadBlocker::terminate(&mut blocker); 
        
           }

Dubious, as an experiment I'm removing it, but @cbrewster reports that @jdm might not be happy about this.

@bors-servo: try


        Auto merge of #18587 - asajeffrey:script-iframe-intial-load-replaces-…

…about-blank, r=<try> Script iframe intial load replaces about blank  When navigating from the initial about:blank, do it with replacement enabled. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18564 - [X] These changes do not require tests because the existing tests do the job    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18587)

bors-servo · 2017-09-21T21:13:40Z

⌛ Trying commit 00af5ba with merge 638c98a...

bors-servo · 2017-09-21T21:49:46Z

💔 Test failed - mac-rel-wpt1


        Don't send two mozbrowserlocationchange events when navigating with r…

…eplacement


        Updated test expectations

asajeffrey · 2017-09-21T22:30:17Z

We were sending a mozbrowserlocationchange event when changing the session history:

servo/components/constellation/constellation.rs

Line 2507 in 29517d5

self.trigger_mozbrowserlocationchange(change.top_level_browsing_context_id);

even though we were traversing the history in the case of a navigation with replacement enabled:

servo/components/constellation/constellation.rs

Line 2467 in 29517d5

self.traverse_to_entry(entry);

which already triggered a location change event:

servo/components/constellation/constellation.rs

Line 2370 in 29517d5

self.trigger_mozbrowserlocationchange(top_level_id);

This was triggering 2 mozbrowserlocationchange events.

asajeffrey · 2017-09-21T22:30:34Z

@bors-servo try

bors-servo · 2017-09-21T22:30:39Z

⌛ Trying commit c6e2460 with merge 03111d5...


        Auto merge of #18587 - asajeffrey:script-iframe-intial-load-replaces-…

…about-blank, r=<try> Script iframe intial load replaces about blank  When navigating from the initial about:blank, do it with replacement enabled. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18564 - [X] These changes do not require tests because the existing tests do the job    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18587)

bors-servo · 2017-09-21T23:44:08Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

cbrewster · 2017-09-25T20:25:02Z

components/constellation/constellation.rs

@@ -2504,7 +2504,10 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>

        // If the navigation is for a top-level browsing context, inform mozbrowser
        if change.browsing_context_id == change.top_level_browsing_context_id {
-            self.trigger_mozbrowserlocationchange(change.top_level_browsing_context_id);
+            // If this is a replacement then we've already triggered the event in traverse_to_entry


Should we just return early when calling traverse_to_entry?

asajeffrey · 2017-11-29T19:23:31Z

On thinking about it, it seems like this PR is just a band-aid, and really we need to have a good look at our whole iframe loading story, with an eye to trying to align it better to the spec.

The navigation from the initial about:blank should be a relacement

5fa2dfe

asajeffrey added the A-content/script label Sep 21, 2017

highfive assigned nox Sep 21, 2017

highfive added the S-awaiting-review label Sep 21, 2017

highfive assigned cbrewster and unassigned nox Sep 21, 2017

highfive added the S-tests-failed label Sep 21, 2017

Don't remove the load blocker when traversing an iframe.

3bcd6c7

asajeffrey force-pushed the asajeffrey:script-iframe-intial-load-replaces-about-blank branch from e527d7f to 00af5ba Sep 21, 2017

highfive removed the S-tests-failed label Sep 21, 2017

highfive added the S-tests-failed label Sep 21, 2017

asajeffrey added 2 commits Sep 21, 2017

Don't send two mozbrowserlocationchange events when navigating with r…

cad9d80

…eplacement

Updated test expectations

Loading status checks…

c6e2460

asajeffrey force-pushed the asajeffrey:script-iframe-intial-load-replaces-about-blank branch from 00af5ba to c6e2460 Sep 21, 2017

highfive removed the S-tests-failed label Sep 21, 2017

asajeffrey added the S-needs-squash label Sep 22, 2017

asajeffrey mentioned this pull request Sep 22, 2017

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

Closed

4 of 4 tasks complete

cbrewster reviewed Sep 25, 2017

View changes

cbrewster added S-needs-code-changes and removed S-awaiting-review labels Oct 13, 2017

asajeffrey closed this Nov 29, 2017

servo / servo

Script iframe intial load replaces about blank #18587

Script iframe intial load replaces about blank #18587

asajeffrey commented Sep 21, 2017 •

edited by SimonSapin

highfive commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

This comment has been minimized.

asajeffrey commented Nov 29, 2017

servo / servo

Join GitHub today

Script iframe intial load replaces about blank #18587

Script iframe intial load replaces about blank #18587

Conversation

asajeffrey commented Sep 21, 2017 • edited by SimonSapin

highfive commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

asajeffrey commented Sep 21, 2017

bors-servo commented Sep 21, 2017

bors-servo commented Sep 21, 2017

This comment has been minimized.

cbrewster Sep 25, 2017

asajeffrey commented Nov 29, 2017

asajeffrey commented Sep 21, 2017 •

edited by SimonSapin