Fix for document salvageable state #20837

gterzian · 2018-05-20T15:53:00Z

I noticed a bug where if the document was un-salavageable due to the presence of beforeunload listeners, this could be overwritten if there were no listeners for unload. This could also have happened if an iframe of the document had no listeners while the parent did.

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

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

This change is

highfive · 2018-05-20T15:53:04Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/document.rs
@fitzgen: components/script/dom/document.rs
@KiChjang: components/script/dom/document.rs

servo-wpt-sync · 2018-05-20T15:53:21Z

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-20T15:56:57Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-20T15:58:59Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

cbrewster

Ah, woops! This change looks correct.
Is there a way to avoid all of the setTimeouts in the test? Sometimes it can lead to intermittent tests with timing issues.

Also, please mention people in a comment rather than the PR body otherwise the mentioned people will get a notification when bors creates a merge commit.

cbrewster · 2018-05-20T21:48:15Z

tests/wpt/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/prompt/010.html

+                         }), 1000);
+}
+
+onload = function() {setTimeout(function() {


Why do we need a setTimeout here?

servo-wpt-sync · 2018-05-21T02:19:19Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T02:22:22Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T02:25:18Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T02:31:12Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

gterzian · 2018-05-21T02:34:01Z

Ok, I was able to remove a few unnecessary timeouts. I also added an empty iframe to make sure the fact that the iframe is salvageable doesn't overwrite the unsalvageable state of the parent...

I noticed, as you mentioned, that the timeouts can be somewhat unreliable, so I've set the one timeout that makes the test pass or fail to a wide margin so that even if the server is slowed down and it takes longer to discard the unsalvageable document, this would not result in an intermittent failure.

servo-wpt-sync · 2018-05-21T02:38:03Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

cbrewster

Tidy:

Checking files for tidiness...
./components/script/dom/document.rs:1668: trailing whitespace

cbrewster · 2018-05-21T03:04:29Z

tests/wpt/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/prompt/004.html

+onload = function() {
+  var iframe = document.getElementsByTagName("iframe")[0]
+  onload = null;
+  iframe.src="004-1.html?" + Math.random();


Is there any reason for Math.random()?

cbrewster · 2018-05-21T03:04:29Z

.../wpt/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/prompt/004-1.html

+onload = function() {
+  if (!parent.loaded) {
+    parent.loaded = true;
+    location="004-2.html?" + Math.random();


Is there any reason for Math.random()?

I think it is to prevent caching.

cbrewster · 2018-05-21T03:04:29Z

.../wpt/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/prompt/004-1.html

+onbeforeunload = function() {
+  onbeforeunload = null;
+  parent.beforeunload_fired = true;
+  setTimeout(function() {


Can we just call a function on parent here that then calls t.done()?

yes, that is done there https://github.com/servo/servo/pull/20837/files#diff-98df125a78b53a9905ea2e52eb1362d5R4
because here the idea is that the timeout never fires because the document is not salvageable hence will be discarded, and the timeout along with it.
(by the way, I didn't quite "invent" this structure myself, I basically copied the same test applied to unload from https://github.com/servo/servo/blob/affcf81ce6156f0796b5e3a4332780135bbd51dd/tests/wpt/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/unload/007.html. Actually I started with the test, then realized it was impossible to make it pass...)

servo-wpt-sync · 2018-05-21T04:20:48Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T04:21:58Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T04:23:05Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T04:45:36Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

gterzian · 2018-05-21T04:46:26Z

components/script/dom/document.rs

+                if !document.salvageable() {
+                   self.salvageable.set(false);
+                }
+                if !can_unload {


ended up changing this, because previously I would check document.salvageable() before calling document.prompt_to_unload(true). Who thought checking booleans could be so hard...

servo-wpt-sync · 2018-05-21T04:47:47Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T06:19:24Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-21T07:09:49Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

servo-wpt-sync · 2018-05-22T09:59:52Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

gterzian · 2018-05-22T10:02:05Z

@cbrewster made a change to the tests, which doesn't change the logic, but hopefully takes care of two flies in one hit in the light of whatwg/html#3646 (comment)

servo-wpt-sync · 2018-05-22T10:09:11Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.


        prevent overwriting of salvageable state in later prompt/unload steps

servo-wpt-sync · 2018-05-22T10:12:22Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#60.

cbrewster · 2018-05-28T19:52:27Z

Thanks!
@bors-servo r+

bors-servo · 2018-05-28T19:52:28Z

📌 Commit 57c0d82 has been approved by cbrewster

bors-servo · 2018-05-28T19:52:35Z

⌛ Testing commit 57c0d82 with merge 2439ab0...


        Auto merge of #20837 - gterzian:tests_for_has_event_listener, r=cbrew…

…ster Fix for document salvageable state I noticed a bug where if the document was un-salavageable due to the presence of `beforeunload` listeners, this could be overwritten if there were no listeners for `unload`. This could also have happened if an iframe of the document had no listeners while the parent did.  ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (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/20837)

bors-servo · 2018-05-28T20:51:17Z

☀️ 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
Approved by: cbrewster
Pushing 2439ab0 to master...


        prevent overwriting of salvageable state in later prompt/unload steps

Upstreamed from servo/servo#20837 [ci skip]

highfive assigned emilio May 20, 2018

highfive added the S-awaiting-review label May 20, 2018

servo-wpt-sync mentioned this pull request May 20, 2018

Fix for document salvageable state jdm/web-platform-tests#60

Merged

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 534965b to 7f36eb8 May 20, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 7f36eb8 to d2109ef May 20, 2018

cbrewster assigned cbrewster and unassigned emilio May 20, 2018

cbrewster reviewed May 20, 2018

View changes

tests/wpt/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/prompt/010.html Outdated

}), 1000);

}

onload = function() {setTimeout(function() {

This comment has been minimized.

Sign in to view

cbrewster May 20, 2018
Member

Why do we need a setTimeout here?

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from d2109ef to 2e55ad3 May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 2e55ad3 to 25819b1 May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 25819b1 to 6e73f24 May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 6e73f24 to 2cda5fc May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 2cda5fc to 32cf8ef May 21, 2018

cbrewster reviewed May 21, 2018

View changes

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 32cf8ef to 42590a2 May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 42590a2 to 67c0575 May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 67c0575 to 255eb13 May 21, 2018

gterzian reviewed May 21, 2018

View changes

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 2663889 to a00051a May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from a00051a to 6385fae May 21, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 6385fae to 42bbe73 May 21, 2018

gterzian mentioned this pull request May 22, 2018

Embedder handling of prompts and alerts #20707

Merged

0 of 6 tasks complete

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 42bbe73 to fb0639d May 22, 2018

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from fb0639d to 29087f4 May 22, 2018

prevent overwriting of salvageable state in later prompt/unload steps

Loading status checks…

57c0d82

gterzian force-pushed the gterzian:tests_for_has_event_listener branch from 29087f4 to 57c0d82 May 22, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels May 28, 2018

highfive removed the S-awaiting-merge label May 28, 2018

gterzian deleted the gterzian:tests_for_has_event_listener branch May 29, 2018

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2018

prevent overwriting of salvageable state in later prompt/unload steps

d0e05e0

Upstreamed from servo/servo#20837 [ci skip]

servo-wpt-sync mentioned this pull request Jun 7, 2018

prevent overwriting of salvageable state in later prompt/unload steps web-platform-tests/wpt#11409

Merged

gterzian mentioned this pull request Jun 13, 2018

[WIP] Make has_listeners_for follow the spec #21044

Open

0 of 5 tasks complete

servo / servo

Fix for document salvageable state #20837

Fix for document salvageable state #20837

gterzian commented May 20, 2018 •

edited by cbrewster

highfive commented May 20, 2018

servo-wpt-sync commented May 20, 2018

servo-wpt-sync commented May 20, 2018

servo-wpt-sync commented May 20, 2018

cbrewster left a comment

This comment has been minimized.

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

gterzian commented May 21, 2018 •

edited

servo-wpt-sync commented May 21, 2018

cbrewster left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

This comment has been minimized.

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 22, 2018

gterzian commented May 22, 2018 •

edited

servo-wpt-sync commented May 22, 2018

servo-wpt-sync commented May 22, 2018

cbrewster commented May 28, 2018

bors-servo commented May 28, 2018

bors-servo commented May 28, 2018

bors-servo commented May 28, 2018

servo / servo

Join GitHub today

Fix for document salvageable state #20837

Fix for document salvageable state #20837

Conversation

gterzian commented May 20, 2018 • edited by cbrewster

highfive commented May 20, 2018

servo-wpt-sync commented May 20, 2018

servo-wpt-sync commented May 20, 2018

servo-wpt-sync commented May 20, 2018

cbrewster left a comment

This comment has been minimized.

cbrewster May 20, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

gterzian commented May 21, 2018 • edited

servo-wpt-sync commented May 21, 2018

cbrewster left a comment

This comment has been minimized.

cbrewster May 21, 2018 • edited

This comment has been minimized.

cbrewster May 21, 2018

This comment has been minimized.

gterzian May 21, 2018 • edited

This comment has been minimized.

cbrewster May 21, 2018

This comment has been minimized.

gterzian May 21, 2018 • edited

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

This comment has been minimized.

gterzian May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 21, 2018

servo-wpt-sync commented May 22, 2018

gterzian commented May 22, 2018 • edited

servo-wpt-sync commented May 22, 2018

servo-wpt-sync commented May 22, 2018

cbrewster commented May 28, 2018

bors-servo commented May 28, 2018

bors-servo commented May 28, 2018

bors-servo commented May 28, 2018

gterzian commented May 20, 2018 •

edited by cbrewster

gterzian commented May 21, 2018 •

edited

cbrewster May 21, 2018 •

edited

gterzian May 21, 2018 •

edited

gterzian May 21, 2018 •

edited

gterzian commented May 22, 2018 •

edited