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

Issue 875735 link

Starred by 1 user
Status: Fixed
Merged: issue 866274
Owner:
timothygu@chromium.org
Closed: Aug 23
Cc:
yukishiino@chromium.org
█ yukiy@google.com
timothygu@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 866274


Participants' hotlists:
document.open


Sign in to add a comment

document.open() may modify a document's URL even in bailout cases

Project Member Reported by yukiy@google.com, Aug 20 
Current implementation does not check Step 6. and 7. of document open steps in accurate order, so Document's url is modified before checking them resulting in making side effect on ignoring opening operations.

Document open steps of HTML standard:
https://html.spec.whatwg.org/C/dynamic-markup-insertion.html#document-open-steps

To solve this issue, it is needed to make new private or protected method Document::Open() that implements opening operation from step 8.
Step 1. to step 7. will be checked in other methods, some Document::open() with different args.

Comment 1 by timothygu@chromium.org, Aug 21

Blocking: 866274
Components: Blink>HTML
Labels: Hotlist-Interop
I have not been able to actually reproduce this bug with Chrome, even though theoretically the bug is there. See related Web Platform Test (which I added this month) testing exactly that: https://wpt.fyi/results/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/bailout-side-effects-synchronous-script.window.html

Interestingly, the test does fail in Safari, whose document.open() code is very similar to ours, so fixing it is certainly worthwhile.

Comment 2 by timothygu@chromium.org, Aug 21

Cc: timothygu@chromium.org
Owner: yukiy@google.com
Status: Started (was: Untriaged)

Comment 3 by yukiy@google.com, Aug 21

Mergedinto: 866274
Status: Duplicate (was: Started)

Comment 5 by timothygu@chromium.org, Aug 22

Summary: document.open() may modify a document's URL even in bailout cases (was: Implement document open steps as defined in HTML standard)
Project Member

Comment 6 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

Comment 7 by timothygu@chromium.org, Aug 23

Labels: M-70
Status: Fixed (was: Started)
►

Sign in to add a comment