New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 866274 link

Starred by 4 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 3
Type: Task


Sign in to add a comment

Document open steps overhaul tracking issue

Project Member Reported by timothygu@chromium.org, Jul 21

Issue description

This is a tracking issue for upcoming changes to document.open() in the HTML Standard. See https://github.com/whatwg/html/issues/3818.
 
Blockedon: 149785 741071 592867
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad4bd885fc65aca019d09231069daefbe7887d21

commit ad4bd885fc65aca019d09231069daefbe7887d21
Author: Timothy Gu <timothygu@chromium.org>
Date: Tue Jul 31 02:43:37 2018

Align executing script bailout in document.open with spec

HTMLDocumentParser::IsExecutingScript() is in fact identical to the
"script nesting level is greater than 0" check in the current HTML
Standard.

The removed check originated from an older version of the HTML Standard
that read:

> If the Document has an active parser that isn't a script-created
> parser, and the insertion point associated with that parser's input
> stream is not undefined (that is, it does point to somewhere in the
> input stream), then the method does nothing.

but that was changed after [1]/[2] to the current text which covers
strictly more cases. Indeed, in the current coverage report the removed
check is never true [3].

The check is expected to stay in its current form in the upcoming
document.open() overhaul.

[1]: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17869
[2]: https://github.com/whatwg/html/commit/7b0112bbfb4923e8f64b0fd1c66d14fbd526fc33
[3]: https://chromium-coverage.appspot.com/reports/577000/linux/chromium/src/third_party/blink/renderer/core/dom/document.cc.html#L3048

Bug: 866274
Change-Id: I5f4547ee62efe45a9831c003854279b3490a08f1
Reviewed-on: https://chromium-review.googlesource.com/1146202
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579290}
[modify] https://crrev.com/ad4bd885fc65aca019d09231069daefbe7887d21/third_party/blink/renderer/core/dom/document.cc

Blockedon: 869910
Blockedon: 68833
Blockedon: 583586
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef638a464133668a549fdae6cb442e974137f572

commit ef638a464133668a549fdae6cb442e974137f572
Author: Timothy Gu <timothygu@chromium.org>
Date: Thu Aug 16 21:54:36 2018

Implement ignore-opens-during-unload counter in Document

The removed test fast/loader/document-destruction-within-unload.html was
originally added in 8809405d796cf3023e26cefd4b06f369eb67f125, to make
sure that a document.write() call that blows away the current page still
works in unload event listeners and blocks any current attempt to
navigate. Since we no longer allow that to happen, the test is obsolete.
In fact, it can be seen as contradictory to
external/wpt/html/browsers/browsing-the-web/unloading-documents/005.html,
which tests that document.open() in an unload event doesn't block
navigation.

Bug:  583586 , 866274
Change-Id: I99b35dd28c97e8603455805b31d49644bc7b23a5
Reviewed-on: https://chromium-review.googlesource.com/1169320
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583836}
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/unloading-documents/003-expected.txt
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/external/wpt/html/browsers/browsing-the-web/unloading-documents/005-expected.txt
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-ignore-opens-during-unload.window-expected.txt
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/ignore-opens-during-unload.window-expected.txt
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/ignore-opens-during-unload.window.js
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/WebKit/LayoutTests/fast/events/alert-in-beforeunload-document-write-expected.txt
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/fast/loader/document-destruction-within-unload-expected.txt
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/fast/loader/document-destruction-within-unload.html
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/fast/loader/resources/document-destruction-within-unload-iframe.html
[delete] https://crrev.com/2a4e2ac108570b33aa72d86a6a0342b24271418b/third_party/WebKit/LayoutTests/fast/loader/resources/document-destruction-within-unload.svg
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/blink/renderer/core/dom/BUILD.gn
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/blink/renderer/core/dom/document.h
[add] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/blink/renderer/core/dom/ignore_opens_during_unload_count_incrementer.h
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/blink/renderer/core/frame/local_frame.cc
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/blink/renderer/core/frame/web_local_frame_impl.cc
[modify] https://crrev.com/ef638a464133668a549fdae6cb442e974137f572/third_party/blink/renderer/core/loader/frame_loader.cc

Blockedon: 437426
Blockedon: 875735
Cc: yukiy@google.com timothygu@chromium.org
 Issue 875735  has been merged into this issue.
Status: Started (was: Assigned)
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e28c32faf9874693a274912fd4b59ba1bee05425

commit e28c32faf9874693a274912fd4b59ba1bee05425
Author: Timothy Gu <timothygu@chromium.org>
Date: Thu Aug 23 04:18:07 2018

Align Document::open() and close() further with the spec

A few checks were moved from open() to open(Document*, ScriptState&) to conform
to the order of execution laid out by the HTML Standard. Web Platform Tests
were added to ensure the fixes stick. In particular, this ensures there is no
side effects in a document.open() call that bailed early.

Document::SetContent() is amended so that write() and close() are not called if
open() bails out early.

Comments were added for better correspondence with the specification.

Bug: 866274,  875735 
Change-Id: I14badd991bb723254f8889ba4073aff19126c7ba
Reviewed-on: https://chromium-review.googlesource.com/1182370
Reviewed-by: Hayato Ito <hayato@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585394}
[modify] https://crrev.com/e28c32faf9874693a274912fd4b59ba1bee05425/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-ignore-opens-during-unload.window.js
[modify] https://crrev.com/e28c32faf9874693a274912fd4b59ba1bee05425/third_party/WebKit/LayoutTests/external/wpt/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-synchronous-script.window.js
[modify] https://crrev.com/e28c32faf9874693a274912fd4b59ba1bee05425/third_party/blink/renderer/core/dom/document.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d566e990909e7ec4f5ada640614434c5d9ab807

commit 4d566e990909e7ec4f5ada640614434c5d9ab807
Author: Timothy Gu <timothygu@chromium.org>
Date: Mon Sep 10 19:08:55 2018

document.open(): Align document aborting behavior with spec

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]: https://github.com/whatwg/html/issues/3447
[2]: https://github.com/whatwg/html/issues/3975
[3]: https://github.com/whatwg/html/pull/3999
[4]: https://github.com/web-platform-tests/wpt/pull/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}
[modify] https://crrev.com/4d566e990909e7ec4f5ada640614434c5d9ab807/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/4d566e990909e7ec4f5ada640614434c5d9ab807/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/4d566e990909e7ec4f5ada640614434c5d9ab807/third_party/blink/renderer/core/loader/frame_loader.cc
[modify] https://crrev.com/4d566e990909e7ec4f5ada640614434c5d9ab807/third_party/blink/renderer/core/loader/frame_loader.h

Blockedon: 884383

Sign in to add a comment