Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTML: document.open() and aborting documents #10789

Merged
merged 1 commit into from
Sep 6, 2018
Merged

HTML: document.open() and aborting documents #10789

merged 1 commit into from
Sep 6, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented May 2, 2018 •
edited by wpt-pr-bot
Loading

For #10773.

This PR (now) tests the new spec text proposed in whatwg/html#3818.

@wpt-pr-bot wpt-pr-bot added the html label May 2, 2018
@wpt-pr-bot wpt-pr-bot requested review from ayg, jdm, jgraham and zqzhang May 2, 2018 13:46
@annevk
Copy link
Member Author

annevk commented May 2, 2018

It appears non-Firefox does not terminate fetches and therefore probably does not run

Abort document.

as required by the HTML Standard.

cc @bzbarsky @jeisinger

@annevk annevk mentioned this pull request May 3, 2018
Merged
@TimothyGu TimothyGu mentioned this pull request Jul 12, 2018
Closed
19 tasks
@wpt-pr-bot wpt-pr-bot requested review from alexshalamov and AmeliaBR and removed request for ayg July 16, 2018 21:06
foolip
foolip previously approved these changes Jul 20, 2018
View reviewed changes
html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort.sub.window.js Outdated
});
client.send();
frame.contentDocument.open();
happened = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the purpose of this to fail the test if frame.contentDocument.open() somehow synchronously aborts the running script? Seems far-fetched, so maybe that's not it...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu suggests there's also the case where error events are fired synchronously, which this would catch.

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort.sub.window.js Outdated

// Since the "unload a document" algorithm is currently executed before "abort
// a document", the following two tests technically test the "unload" algorithm
// since that is where "make disappear a WebSocket object" is called, rather
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: "make disappear" is a brilliant name for some steps :)

@TimothyGu TimothyGu self-assigned this Jul 31, 2018
TimothyGu added a commit to TimothyGu/html that referenced this pull request Aug 14, 2018
@TimothyGu @annevk
Remove document.open() realm creation
7234c89 
Based on the work by Anne van Kesteren in whatwg#3651, but without the parts
concerning the session history. Changes to that area will come later
(see whatwg#3818).

Tests: web-platform-tests/wpt#10773
Tests: web-platform-tests/wpt#10778
Tests: web-platform-tests/wpt#10789
Tests: web-platform-tests/wpt#10815
Tests: web-platform-tests/wpt#10818

Fixes whatwg#1698.
Fixes whatwg#3286.
Fixes whatwg#3306.
Closes whatwg#3665.

Co-authored-by: Anne van Kesteren <[email protected]>
@TimothyGu TimothyGu dismissed foolip’s stale review August 14, 2018 19:03

Updated PR flipped the reference conditions.

TimothyGu added a commit to TimothyGu/html that referenced this pull request Aug 29, 2018
@TimothyGu
document.open(): only abort when there is a navigation
12e5a09 
See investigation in whatwg#3818.

Tests: web-platform-tests/wpt#10789
@TimothyGu TimothyGu force-pushed the annevk/document-open-steps-abort branch 3 times, most recently from a6a862c to ae9ad45 Compare August 31, 2018 16:05
TimothyGu added a commit to TimothyGu/html that referenced this pull request Sep 4, 2018
@TimothyGu
document.open(): only abort when there is a navigation
90dffec 
This implements the "ideal 2" plan in whatwg#3651, which is found to be
compatible with the existing Chrome test suite.

Closes whatwg#3651.
Fixes whatwg#3975.

Tests: web-platform-tests/wpt#10789
@TimothyGu TimothyGu mentioned this pull request Sep 4, 2018
Merged
@TimothyGu TimothyGu force-pushed the annevk/document-open-steps-abort branch from 6353140 to b623d1b Compare September 5, 2018 14:23
domenic
domenic reviewed Sep 6, 2018
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions, but overall good to go. The only thing I'd really like done is moving .src assignments after .onload assignments; the rest is up to your judgment.

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/resources/meta-refresh.py Show resolved Hide resolved
...bappapis/dynamic-markup-insertion/opening-the-input-stream/abort-refresh-immediate.window.js Outdated
async_test(t => {
const frame = document.body.appendChild(document.createElement("iframe"));
t.add_cleanup(() => frame.remove());
frame.src = "resources/meta-refresh.py?0";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make me more comfortable if you put the .src assignments after the .onload assignments in all cases. I'm not sure what exactly is allowed per spec, but in the past we've tried to make tests follow that order.

...dynamic-markup-insertion/opening-the-input-stream/abort-refresh-multisecond-header.window.js Show resolved Hide resolved
html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort.sub.window.js Outdated
}, "document.open() does not abort documents that are not navigating (image loading)");

async_test(t => {
const __SERVER__NAME = "{{host}}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why __ prefix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to use the convention in https://github.com/web-platform-tests/wpt/blob/master/websockets/websocket.sub.js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could change the websocket variables.

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/resources/slow.py Outdated
@@ -0,0 +1,5 @@
import time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe best to dedupe this with https://github.com/web-platform-tests/wpt/blob/master/html/semantics/scripting-1/the-script-element/resources/slow.py into /common?=

html/webappapis/dynamic-markup-insertion/opening-the-input-stream/resources/http-refresh.py Outdated Show resolved Hide resolved
@wpt-pr-bot wpt-pr-bot requested a review from deniak September 6, 2018 16:12
@TimothyGu TimothyGu force-pushed the annevk/document-open-steps-abort branch 2 times, most recently from b1aeb1b to fa75e02 Compare September 6, 2018 17:37
@TimothyGu TimothyGu force-pushed the annevk/document-open-steps-abort branch from fa75e02 to 0a04996 Compare September 6, 2018 18:24
@TimothyGu TimothyGu merged commit 2c6612a into master Sep 6, 2018
@TimothyGu TimothyGu deleted the annevk/document-open-steps-abort branch September 6, 2018 18:42
domenic pushed a commit to whatwg/html that referenced this pull request Sep 6, 2018
@TimothyGu @domenic
document.open(): only abort when there is a navigation
1ca520a 
This implements the "ideal 2" plan in #3975, which was found to be
compatible with the existing Chrome test suite while being reasonably
straightforward.

Closes #3651.
Fixes #3975.

Tests: web-platform-tests/wpt#10789
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 10, 2018
@TimothyGu
document.open(): Align document aborting behavior with spec
4d566e9 
Currently, we have different behaviors for the "having a provisional
document loader" state versus the "having a queued navigation" state. In
the first case, we call FrameLoader::StopAllLoaders(), which cancels the
ongoing navigation as well as fetches on the current page (e.g.
XMLHttpRequest). In the second, we merely cancel the task to navigate,
but do NOT cancel fetches.

Indeed, it is recognized that the spec is currently unclear about
canceling queued navigation vs. direct navigation (see [1]). However, it
is worth noting that Chrome does not always follow the spec with this
distinction in the first place (through location.href, for example,
which queues a navigation task in Chrome but navigates directly in
spec).

Additionally, since even the current code cancels navigation in both
circumstances (the only disagreement being if peripheral fetches are
also canceled), we see no reason to have an inconsistency in this regard
(see [2]).

This CL now always calls FrameLoader::StopAllLoaders(), for both when we
have a provisional loader and when we have a queued navigation, thus
ridding ourselves of the inconsistency.

By doing so, we implement the "ideal 2" plan laid out in [2], which
recently became part of the HTML Standard in [3]. Tests for this new
behavior (which this CL fully passes) are in [4], which was imported
into our tree by the WPT Importer bot, whose expectations this CL now
change.

[1]: whatwg/html#3447
[2]: whatwg/html#3975
[3]: whatwg/html#3999
[4]: web-platform-tests/wpt#10789

Bug: 866274
Change-Id: I4e3ffac6b7c07bc8da812f6f210ab5d6933bdfd1
Reviewed-on: https://chromium-review.googlesource.com/1195837
Commit-Queue: Timothy Gu <[email protected]>
Reviewed-by: Nate Chapin <[email protected]>
Reviewed-by: Kent Tamura <[email protected]>
Cr-Commit-Position: refs/heads/master@{#590011}
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
@TimothyGu @mustaqahmed
document.open(): only abort when there is a navigation
c405145 
This implements the "ideal 2" plan in whatwg#3975, which was found to be
compatible with the existing Chrome test suite while being reasonably
straightforward.

Closes whatwg#3651.
Fixes whatwg#3975.

Tests: web-platform-tests/wpt#10789
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
@TimothyGu @mustaqahmed
document.open(): only abort when there is a navigation
d82d154 
This implements the "ideal 2" plan in whatwg#3975, which was found to be
compatible with the existing Chrome test suite while being reasonably
straightforward.

Closes whatwg#3651.
Fixes whatwg#3975.

Tests: web-platform-tests/wpt#10789
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@foolip foolip foolip left review comments

@domenic domenic domenic approved these changes

@jdm jdm Awaiting requested review from jdm

@jgraham jgraham Awaiting requested review from jgraham

@zqzhang zqzhang Awaiting requested review from zqzhang

@gsnedders gsnedders Awaiting requested review from gsnedders

@kojiishi kojiishi Awaiting requested review from kojiishi

@plinss plinss Awaiting requested review from plinss

@sideshowbarker sideshowbarker Awaiting requested review from sideshowbarker

@zcorpan zcorpan Awaiting requested review from zcorpan

@deniak deniak Awaiting requested review from deniak

Assignees

@TimothyGu TimothyGu

Labels
assets html
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@annevk @zcorpan @foolip @domenic @TimothyGu @wpt-pr-bot