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

Unclear what exactly should be aborted in document.open() #3975

Closed
TimothyGu opened this issue Aug 30, 2018 · 1 comment
Closed

Unclear what exactly should be aborted in document.open() #3975

TimothyGu opened this issue Aug 30, 2018 · 1 comment

Comments

@TimothyGu
Copy link
Member

TimothyGu commented Aug 30, 2018

Chrome (and probably Safari):

Here's what Blink and on a large part WebKit do when document.open() gets called

  1. If navigation is ongoing
    1. window.stop() steps, which are:
      1. Recurse to nested BCs (not in spec except for Abort step)
      2. If navigation is ongoing, cancel that navigation (including firing load event if applicable)
      3. Abort, which is:
        1. Cancel ongoing fetches
        2. Abort the parser (fires two readystatechange events if parser is active, which could maybe be observable given long pages that do setTimeout(() => document.open(), 0) near the top?)
      4. Cancel queued navigations (not in spec, Clarify how to cancel a navigation #3447)
  2. Cancel queued navigations (this handles cases like setting location.href, which in Blink/WebKit queues a task to navigate instead of navigating directly)

Firefox

not sure, but it does cancel navigation to some extent

Spec:

Currently, document.open() in spec only does:

  1. Abort
    1. Cancel fetches, including navigation
    2. Abort parser

I tried this out in Chrome (with canceling ongoing and queued navigation), it breaks a lot of tests.

Ideal 1:

This is basically Chrome's current behavior without canceling ongoing fetches.

  1. Cancel ongoing and queued navigation (would probably be merged by fixing Clarify how to cancel a navigation #3447)
  2. Abort the parser (necessary because step 16 will create a new one; maybe move to right before step 16)

Reasoning versus Chrome: it's weird that abort, which messes with fetches (not just navigations), is conditional on navigation being ongoing rather than queued.

Ideal 2:

  1. If navigation is ongoing [or queued], run the window.stop() steps:
    1. Abort, which is:
      1. Cancel ongoing fetches
      2. Abort the parser
    2. Cancel ongoing [and queued] navigation

Bracketed wording would probably be best addressed by fixing #3447)

Reasoning: more straightforward, and mostly reuses existing primitives.

Other

  • How much of this, if any, recurses into descendant BCs? (Abort does that per spec. Chrome does that only if there is an ongoing navigation)
  • We should probably move all of this after erasing the event listeners to make it less observable (e.g. to avoid the weird readystatechange issues in long documents when aborting the parser)

/cc @bzbarsky @zetafunction @natechapin

@TimothyGu
Copy link
Member Author

For what it's worth, "Ideal 2" when implemented in Chromium passes the entire test suite.

TimothyGu added a commit to TimothyGu/html that referenced this issue Sep 4, 2018
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
domenic pushed a commit that referenced this issue Sep 6, 2018
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 issue 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 <timothygu@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590011}
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
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 issue Feb 15, 2019
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
webkit-early-warning-system pushed a commit to cdumez/WebKit that referenced this issue Sep 20, 2022
https://bugs.webkit.org/show_bug.cgi?id=245407

Reviewed by Youenn Fablet.

document.open() should abort all loads when the document is navigating or there is a queued navigation:
- whatwg/html#3975

This aligns our behavior with Blink and Gecko, as they already already passing those WPT tests.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-refresh-immediate.window-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-while-navigating.window-expected.txt:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::open):
* Source/WebCore/loader/NavigationScheduler.cpp:
(WebCore::NavigationScheduler::hasQueuedNavigation const):
* Source/WebCore/loader/NavigationScheduler.h:

Canonical link: https://commits.webkit.org/254699@main
aperezdc pushed a commit to WebKit/WebKit that referenced this issue Sep 22, 2022
…ocument is navigating

https://bugs.webkit.org/show_bug.cgi?id=245407

Reviewed by Youenn Fablet.

document.open() should abort all loads when the document is navigating or there is a queued navigation:
- whatwg/html#3975

This aligns our behavior with Blink and Gecko, as they already already passing those WPT tests.

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-refresh-immediate.window-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-while-navigating.window-expected.txt:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::open):
* Source/WebCore/loader/NavigationScheduler.cpp:
(WebCore::NavigationScheduler::hasQueuedNavigation const):
* Source/WebCore/loader/NavigationScheduler.h:

Canonical link: https://commits.webkit.org/254699@main

(cherry picked from commit 952f3c7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants