Skip to content

Commit

Permalink
document.open(): Align document aborting behavior with spec
Browse files Browse the repository at this point in the history
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}
  • Loading branch information
TimothyGu authored and Commit Bot committed Sep 10, 2018
1 parent e5b5efa commit 4d566e9
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 15 deletions.
2 changes: 0 additions & 2 deletions third_party/WebKit/LayoutTests/TestExpectations
Original file line number Diff line number Diff line change
Expand Up @@ -2807,8 +2807,6 @@ crbug.com/626703 external/wpt/html/semantics/interactive-elements/commands/legen
crbug.com/626703 external/wpt/html/semantics/interactive-elements/commands/legend/no-fieldset-parent-manual.html [ Skip ]
crbug.com/626703 external/wpt/html/semantics/interactive-elements/commands/legend/focusable-legend-manual.html [ Skip ]
crbug.com/626703 external/wpt/css/css-scrollbars/textarea-scrollbar-width-none.html [ Failure ]
crbug.com/626703 external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-refresh-immediate.window.html [ Timeout ]
crbug.com/626703 external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/abort-while-navigating.window.html [ Timeout ]
crbug.com/880983 external/wpt/css/css-masking/clip-path/clip-path-path-002.html [ Failure ]
crbug.com/880983 external/wpt/css/css-masking/clip-path/clip-path-path-001.html [ Failure ]
crbug.com/432153 external/wpt/css/css-masking/mask-image/mask-image-url-local-mask.html [ Failure ]
Expand Down
19 changes: 15 additions & 4 deletions third_party/blink/renderer/core/dom/document.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3098,11 +3098,22 @@ void Document::open() {
if (ScriptableDocumentParser* parser = GetScriptableDocumentParser())
DCHECK(!parser->IsParsing() || !parser->IsExecutingScript());

// Abort |document|.
// If |document| has a browsing context and there is an existing attempt to
// navigate |document|'s browsing context, then stop document loading given
// |document|.
//
// TODO(timothygu): We are only aborting the document if there is a
// provisional navigation, unlike the spec.
if (frame_ && frame_->Loader().HasProvisionalNavigation()) {
// As noted in the spec and https://github.com/whatwg/html/issues/3975, we
// want to treat ongoing navigation and queued navigation the same way.
// However, we don't want to consider navigations scheduled too much into the
// future through Refresh headers or a <meta> refresh pragma to be a current
// navigation. Thus, we cut it off with IsNavigationScheduledWithin(0).
//
// This also prevents window.open(url) -- eg window.open("about:blank") --
// from blowing away results from a subsequent window.document.open /
// window.document.write call.
if (frame_ &&
(frame_->Loader().HasProvisionalNavigation() ||
frame_->GetNavigationScheduler().IsNavigationScheduledWithin(0))) {
frame_->Loader().StopAllLoaders();
// Navigations handled by the client should also be cancelled.
if (frame_ && frame_->Client())
Expand Down
6 changes: 0 additions & 6 deletions third_party/blink/renderer/core/loader/frame_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -345,12 +345,6 @@ void FrameLoader::DidExplicitOpen() {
progress_tracker_->ProgressStarted();
}
}

// Prevent window.open(url) -- eg window.open("about:blank") -- from blowing
// away results from a subsequent window.document.open / window.document.write
// call. Canceling redirection here works for all cases because document.open
// implicitly precedes document.write.
frame_->GetNavigationScheduler().Cancel();
}

// This is only called by ScriptController::executeScriptIfJavaScriptURL and
Expand Down
12 changes: 9 additions & 3 deletions third_party/blink/renderer/core/loader/frame_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,15 @@ class CORE_EXPORT FrameLoader final {
bool has_event,
std::unique_ptr<WebDocumentLoader::ExtraData> extra_data = nullptr);

// Warning: stopAllLoaders can and will detach the LocalFrame out from under
// you. All callers need to either protect the LocalFrame or guarantee they
// won't in any way access the LocalFrame after stopAllLoaders returns.
// This runs the "stop document loading" algorithm in HTML:
// https://html.spec.whatwg.org/C/browsing-the-web.html#stop-document-loading
// Note, this function only cancels ongoing navigation handled through
// FrameLoader. You might also want to call
// LocalFrameClient::AbortClientNavigation() if appropriate.
//
// Warning: StopAllLoaders() may detach the LocalFrame to which this
// FrameLoader belongs. Callers need to be careful about checking the
// existence of the frame after StopAllLoaders() returns.
void StopAllLoaders();

void ReplaceDocumentWhileExecutingJavaScriptURL(const String& source,
Expand Down

0 comments on commit 4d566e9

Please sign in to comment.