[WIP] Make subsequent about:blank loads async #14764

cbrewster · 2016-12-28T19:02:39Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14856 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-12-28T19:02:42Z

Heads up! This PR modifies the following files:

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

highfive · 2016-12-28T19:02:43Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

cbrewster · 2016-12-28T19:03:06Z

r? @jdm
cc @asajeffrey
Should a test for this be in wpt or mozilla?

jdm · 2016-12-28T19:11:01Z

Depends what it's testing, really :) If you can write a test that checks behaviour that the spec describes, then it belongs in wpt.

@bors-servo: try

bors-servo · 2016-12-28T19:11:05Z

⌛ Trying commit 6d42d53 with merge 265b72b...


        Auto merge of #14764 - cbrewster:about_blank_fix, r=<try>

Only send ScriptLoadedAboutBlankInIFrame on initial BC creation  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #13716 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14764)

cbrewster · 2016-12-28T19:13:23Z

@jdm an easy test is to create an iframe with some src (can be anything), then set src to about:blank and then use the History API to traverse backwards with the idea that this won't cause a crash.

jdm · 2016-12-28T20:04:58Z

Whenever possible, have the test verify expected outcomes beyond just "does this crash", since that will make the test more meaningful to both us and other browsers.

bors-servo · 2016-12-28T20:09:27Z

💔 Test failed - mac-rel-wpt2

jdm · 2016-12-28T20:16:03Z

  ▶ ERROR [expected OK] /webstorage/event_body_attribute.html
  └   → iframe.contentWindow is null

  ▶ Unexpected subtest result in /webstorage/event_body_attribute.html:
  │ FAIL [expected PASS] sessionStorage mutations fire StorageEvents that are caught by the event listener specified as an attribute on the body.
  │   → null has no properties
  │ FAIL [expected PASS] localStorage mutations fire StorageEvents that are caught by the event listener specified as an attribute on the body.
  │   → null has no properties
  │ 
  │ step0@http://web-platform.test:8000/webstorage/event_body_attribute.js:15:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ ERROR [expected OK] /webstorage/event_basic.html
  └   → iframe.contentWindow is null

  ▶ ERROR [expected OK] /webstorage/event_setattribute.html
  └   → iframe.contentWindow is null

  ▶ Unexpected subtest result in /webstorage/event_setattribute.html:
  │ FAIL [expected PASS] sessionStorage mutations fire StorageEvents that are caught by the event listener attached via setattribute.
  │   → null has no properties
  │ FAIL [expected PASS] localStorage mutations fire StorageEvents that are caught by the event listener attached via setattribute.
  │   → null has no properties
  │ 
  │ step0@http://web-platform.test:8000/webstorage/event_setattribute.js:15:13
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1406:20
  └ Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1430:20

  ▶ ERROR [expected OK] /webstorage/event_case_sensitive.html
  └   → iframe.contentWindow is null

cbrewster · 2016-12-28T23:05:28Z

After some digging I found that the issue is that whenever src is set and then contentDocument is called right after, there is a race condition and contentDocument will return null because the Document hasn't been created in the script thread yet.

These tests were previously passing because they set src to about:blank. This was a special case because navigate_or_reload_child_browsing_context checked to see if the Url is about:blank in which case it believe that it is the initial about:blank load which will cause the load to be synchronous when really it should only do the synchronous load when the iframe is initially loaded.

I am not entirely sure the best way to fix the race condition but I think it would be ok to update the test expectations here and fix the race condition in a separate PR. Unless all about:blank loads should be synchronous (in which case we just need to fix a couple things on the constellation side of things).

Edit: The plan is to wait until we figure out how to fix the tests.

Ms2ger

Do squash.

Ms2ger · 2016-12-29T10:07:38Z

components/script/dom/htmliframeelement.rs

@@ -75,6 +75,13 @@ enum ProcessingMode {
    NotFirstTime,
 }

+#[derive(PartialEq)]
+pub enum NavigationType {
+    SynchronousAboutBlank,


Maybe InitialAboutBlank.

bors-servo · 2016-12-29T10:34:18Z

☔ The latest upstream changes (presumably #14647) made this pull request unmergeable. Please resolve the merge conflicts.

cbrewster · 2016-12-29T18:25:46Z

Checklist

Subsequent about:blank loads should be async; however, they should be treated as same-origin navigations (share the script thread).
Iframe's should not update their current PipelineId until the Document associated with the new Pipeline has matured.

cbrewster · 2016-12-29T20:42:20Z

This should be ready for review
@bors-servo try

bors-servo · 2017-04-05T07:35:40Z

☔ The latest upstream changes (presumably #16268) made this pull request unmergeable. Please resolve the merge conflicts.


        script: only initial about:blank should be loaded synchronously

Create all about:blank loads in script


        constellation: cancel any in progress loads when navigating


        update pipeline on iframe after document is created


        Only allow active documents to trigger iframe load event

cbrewster · 2017-04-13T21:31:29Z

@bors-servo try

bors-servo · 2017-04-13T21:31:33Z

⌛ Trying commit 499843b with merge 21763ba...


        Auto merge of #14764 - cbrewster:about_blank_fix, r=<try>

[WIP] Make subsequent about:blank loads async  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14856 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14764)

bors-servo · 2017-04-13T21:32:33Z

💔 Test failed - linux-rel-wpt


        Store whether a pipeline is the initial about:blank

cbrewster · 2017-04-13T22:25:59Z

@bors-servo try

bors-servo · 2017-04-13T22:26:03Z

⌛ Trying commit ce918c0 with merge dbe102d...


        Auto merge of #14764 - cbrewster:about_blank_fix, r=<try>

[WIP] Make subsequent about:blank loads async  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14856 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14764)

bors-servo · 2017-04-13T22:47:16Z

💔 Test failed - linux-rel-css

cbrewster · 2017-04-18T01:36:57Z

Superseded by #16506

highfive assigned nox Dec 28, 2016

highfive added the S-awaiting-review label Dec 28, 2016

highfive assigned jdm and unassigned nox Dec 28, 2016

highfive added the S-tests-failed label Dec 28, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 28, 2016

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Dec 28, 2016

cbrewster mentioned this pull request Dec 28, 2016

contentWindow is null right after iframe src is set #14767

Closed

Ms2ger requested changes Dec 29, 2016

View changes

highfive added the S-needs-rebase label Dec 29, 2016

cbrewster changed the title ~~Only send ScriptLoadedAboutBlankInIFrame on initial BC creation~~ Make subsequent about:blank loads async Dec 29, 2016

cbrewster force-pushed the cbrewster:about_blank_fix branch from 9e34ef7 to bed1ef8 Dec 29, 2016

jdm removed the S-needs-rebase label Dec 29, 2016

highfive added the S-tests-failed label Apr 5, 2017

highfive added the S-needs-rebase label Apr 5, 2017

cbrewster removed the S-awaiting-review label Apr 10, 2017

cbrewster added 4 commits Jan 5, 2017

script: only initial about:blank should be loaded synchronously

1a2c30e

Create all about:blank loads in script

constellation: cancel any in progress loads when navigating

ccb3c21

update pipeline on iframe after document is created

8edd357

Only allow active documents to trigger iframe load event

e056f86

cbrewster force-pushed the cbrewster:about_blank_fix branch from 708130c to 92cf190 Apr 13, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Apr 13, 2017

cbrewster force-pushed the cbrewster:about_blank_fix branch from 92cf190 to 499843b Apr 13, 2017

highfive added the S-tests-failed label Apr 13, 2017

Store whether a pipeline is the initial about:blank

Loading status checks…

ce918c0

cbrewster force-pushed the cbrewster:about_blank_fix branch from 499843b to ce918c0 Apr 13, 2017

highfive removed the S-tests-failed label Apr 13, 2017

highfive added the S-tests-failed label Apr 13, 2017

cbrewster removed S-awaiting-review S-needs-rebase labels Apr 14, 2017

cbrewster mentioned this pull request Apr 17, 2017

Better handling of about:blank and iframes #14856

Closed

cbrewster closed this Apr 18, 2017

servo / servo

[WIP] Make subsequent about:blank loads async #14764

[WIP] Make subsequent about:blank loads async #14764

cbrewster commented Dec 28, 2016 •

edited

highfive commented Dec 28, 2016

highfive commented Dec 28, 2016

cbrewster commented Dec 28, 2016

jdm commented Dec 28, 2016

bors-servo commented Dec 28, 2016

cbrewster commented Dec 28, 2016

jdm commented Dec 28, 2016

bors-servo commented Dec 28, 2016

jdm commented Dec 28, 2016

cbrewster commented Dec 28, 2016 •

edited

Ms2ger left a comment

This comment has been minimized.

This comment has been minimized.

bors-servo commented Dec 29, 2016

cbrewster commented Dec 29, 2016 •

edited

cbrewster commented Dec 29, 2016

bors-servo commented Apr 5, 2017

cbrewster commented Apr 13, 2017

bors-servo commented Apr 13, 2017

bors-servo commented Apr 13, 2017

cbrewster commented Apr 13, 2017

bors-servo commented Apr 13, 2017

bors-servo commented Apr 13, 2017

cbrewster commented Apr 18, 2017

servo / servo

Join GitHub today

[WIP] Make subsequent about:blank loads async #14764

[WIP] Make subsequent about:blank loads async #14764

Conversation

cbrewster commented Dec 28, 2016 • edited

highfive commented Dec 28, 2016

highfive commented Dec 28, 2016

cbrewster commented Dec 28, 2016

jdm commented Dec 28, 2016

bors-servo commented Dec 28, 2016

cbrewster commented Dec 28, 2016

jdm commented Dec 28, 2016

bors-servo commented Dec 28, 2016

jdm commented Dec 28, 2016

cbrewster commented Dec 28, 2016 • edited

Ms2ger left a comment

This comment has been minimized.

Ms2ger Dec 29, 2016

This comment has been minimized.

cbrewster Dec 29, 2016

bors-servo commented Dec 29, 2016

cbrewster commented Dec 29, 2016 • edited

cbrewster commented Dec 29, 2016

bors-servo commented Apr 5, 2017

cbrewster commented Apr 13, 2017

bors-servo commented Apr 13, 2017

bors-servo commented Apr 13, 2017

cbrewster commented Apr 13, 2017

bors-servo commented Apr 13, 2017

bors-servo commented Apr 13, 2017

cbrewster commented Apr 18, 2017

cbrewster commented Dec 28, 2016 •

edited

cbrewster commented Dec 28, 2016 •

edited

cbrewster commented Dec 29, 2016 •

edited