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.

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

document.open(): only abort when there is a navigation #3999

Merged
merged 5 commits into from
Sep 6, 2018

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Sep 4, 2018

This implements the "ideal 2" plan in #3975, which is found to be compatible with the existing Chrome test suite.

Closes #3651.
Fixes #3975.

Tests: web-platform-tests/wpt#10789


/browsing-the-web.html ( diff )
/dynamic-markup-insertion.html ( diff )
/window-object.html ( diff )

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
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.

Some simplifications possible

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
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.

LGTM with potential improvement. This cleaned up nicely.

source Outdated Show resolved Hide resolved
TimothyGu pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 6, 2018
@domenic domenic merged commit 1ca520a into whatwg:master Sep 6, 2018
@TimothyGu TimothyGu deleted the document-open-abort branch September 6, 2018 19:35
domenic pushed a commit that referenced this pull request Sep 7, 2018
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 10, 2018
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}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 12, 2018
…ents, a=testonly

Automatic update from web-platform-testsHTML: document.open() and aborting documents (#10789)

For whatwg/html#3999.

Co-authored-by: Anne van Kesteren <[email protected]>
--

wpt-commits: 2c6612a9b0f67c95fc8cc772cfecc2c1d435d75d
wpt-pr: 10789


--HG--
rename : testing/web-platform/tests/html/semantics/scripting-1/the-script-element/resources/slow.py => testing/web-platform/tests/common/slow.py
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Sep 12, 2018
…ents, a=testonly

Automatic update from web-platform-testsHTML: document.open() and aborting documents (#10789)

For whatwg/html#3999.

Co-authored-by: Anne van Kesteren <[email protected]>
--

wpt-commits: 2c6612a9b0f67c95fc8cc772cfecc2c1d435d75d
wpt-pr: 10789
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
mustaqahmed pushed a commit to mustaqahmed/html that referenced this pull request Feb 15, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…ents, a=testonly

Automatic update from web-platform-testsHTML: document.open() and aborting documents (#10789)

For whatwg/html#3999.

Co-authored-by: Anne van Kesteren <annevkannevk.nl>
--

wpt-commits: 2c6612a9b0f67c95fc8cc772cfecc2c1d435d75d
wpt-pr: 10789

UltraBlame original commit: b5ebc86c7bce94cf27b6d03c84ca8282ca285519
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…ents, a=testonly

Automatic update from web-platform-testsHTML: document.open() and aborting documents (#10789)

For whatwg/html#3999.

Co-authored-by: Anne van Kesteren <annevkannevk.nl>
--

wpt-commits: 2c6612a9b0f67c95fc8cc772cfecc2c1d435d75d
wpt-pr: 10789

UltraBlame original commit: b5ebc86c7bce94cf27b6d03c84ca8282ca285519
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…ents, a=testonly

Automatic update from web-platform-testsHTML: document.open() and aborting documents (#10789)

For whatwg/html#3999.

Co-authored-by: Anne van Kesteren <annevkannevk.nl>
--

wpt-commits: 2c6612a9b0f67c95fc8cc772cfecc2c1d435d75d
wpt-pr: 10789

UltraBlame original commit: b5ebc86c7bce94cf27b6d03c84ca8282ca285519
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants