Bug 187319 - document.open and document.write must throw while the HTML parser is synchronously constructing a custom element
Summary: document.open and document.write must throw while the HTML parser is synchron...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL: https://html.spec.whatwg.org/multipag...
Keywords: InRadar
Depends on: 188336 188390
Blocks: 154907
  Show dependency treegraph
 
Reported: 2018-07-04 02:43 PDT by Frédéric Wang (:fredw)
Modified: 2018-08-13 09:14 PDT (History)
12 users (show)

See Also:


Attachments
Fixes the bug (32.50 KB, patch)
2018-08-06 22:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (32.72 KB, patch)
2018-08-06 22:41 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (32.14 KB, patch)
2018-08-07 14:04 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (32.03 KB, patch)
2018-08-07 14:47 PDT, Ryosuke Niwa
rniwa: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2018-07-04 02:43:06 PDT
"Document objects have a throw-on-dynamic-markup-insertion counter, which is used in conjunction with the create an element for the token algorithm to prevent custom element constructors from being able to use document.open(type, replace), document.close(), and document.write() when they are invoked by the parser. Initially, the counter must be set to zero."

We have FIXMEs comments but no throw-on-dynamic-markup-insertion counter and the calls are always allowed.
Comment 1 Timothy Gu 2018-07-17 12:22:32 PDT
WPT is available after https://github.com/web-platform-tests/wpt/pull/12037.
Comment 2 Frédéric Wang (:fredw) 2018-07-23 08:57:21 PDT
Additional note: We don't follow steps 6 and 9 of https://html.spec.whatwg.org/#create-an-element-for-the-token so the throw-on-dynamic-markup-insertion counter is always zero anyway.
Comment 3 Radar WebKit Bug Importer 2018-08-01 22:42:08 PDT
<rdar://problem/42843012>
Comment 4 Ryosuke Niwa 2018-08-06 20:08:33 PDT
Also see https://github.com/whatwg/html/pull/1673
Comment 5 Ryosuke Niwa 2018-08-06 22:38:43 PDT
Created attachment 346686 [details]
Fixes the bug
Comment 6 Build Bot 2018-08-06 22:40:16 PDT
Attachment 346686 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ThrowOnDynamicMarkupInsertionCountIncrementer.h:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Ryosuke Niwa 2018-08-06 22:41:25 PDT
Created attachment 346687 [details]
Fixes the bug
Comment 8 Frédéric Wang (:fredw) 2018-08-07 01:24:37 PDT
Comment on attachment 346687 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=346687&action=review

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:215
> +            // Prevent document.open/write during reactions by allocating the incrementer before the reactions stack.

I would have put the comment before the incrementer variable.

> LayoutTests/ChangeLog:10
> +        doesn't test nearly as many edge cases.

I think it would be nice to export the new tests to upstream WPT (nice set of tests BTW!)

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-construct.html:1
> +<!DOCTYPE html>

I think content was copied from throw-on-dynamic-markup-insertion-counter-reactions.html so see my review comments in the other file, since they apply here too.

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-construct.html:114
> +async function custom_element_reactions_in_parser(test, call_function)

This function does not belong to that file, right?

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:61
> +}, 'document.open("about:blank") must NOT throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');

I don't understand that test without investigating more the spec. Can you please explain?

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:73
> +}, 'document.write must throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');

Maybe you want to use parenthesis or specify arguments in the text, for consistency with other cases? (same for writeln below)

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:76
> +    const result = await custom_element_reactions_in_parser(this, (document) => document.write('<b>some text</b>'));

Should be writeln I guess.

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:105
> +}, 'document.write must throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');

must not throw and for another document

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:109
> +    const result = await custom_element_reactions_in_parser(this, (document) => another_window.document.write('<b>some text</b>'));

Again, I guess it's writeln

> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:112
> +}, 'document.writeln must throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');

must not throw and for another document
Comment 9 Ryosuke Niwa 2018-08-07 13:58:55 PDT
(In reply to Frédéric Wang (:fredw) from comment #8)
> Comment on attachment 346687 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346687&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:215
> > +            // Prevent document.open/write during reactions by allocating the incrementer before the reactions stack.
> 
> I would have put the comment before the incrementer variable.

Fixed.

> > LayoutTests/ChangeLog:10
> > +        doesn't test nearly as many edge cases.
> 
> I think it would be nice to export the new tests to upstream WPT (nice set
> of tests BTW!)

Fixed.

> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-construct.html:1
> > +<!DOCTYPE html>
> 
> I think content was copied from
> throw-on-dynamic-markup-insertion-counter-reactions.html so see my review
> comments in the other file, since they apply here too.
> 
> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-construct.html:114
> > +async function custom_element_reactions_in_parser(test, call_function)
> 
> This function does not belong to that file, right?

Oops, removed.

> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:61
> > +}, 'document.open("about:blank") must NOT throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');
> 
> I don't understand that test without investigating more the spec. Can you
> please explain?

Oh, oops, this should read as "document.open(URL)" since I'm not using about:blank anymore. This is a version of document.open which behaves like window.open:
https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-open-window

Added a comment above this test case.

> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:73
> > +}, 'document.write must throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');
> 
> Maybe you want to use parenthesis or specify arguments in the text, for
> consistency with other cases? (same for writeln below)

I only put () to other test cases because they're two variants of document.open. The one which takes optional type argument and the one which takes URL, target, and features. That's not the case here for document.write.

> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:76
> > +    const result = await custom_element_reactions_in_parser(this, (document) => document.write('<b>some text</b>'));
> 
> Should be writeln I guess.

Oops, fixed.

> 
> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:105
> > +}, 'document.write must throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');
> 
> must not throw and for another document

Fixed.

> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:109
> > +    const result = await custom_element_reactions_in_parser(this, (document) => another_window.document.write('<b>some text</b>'));
> 
> Again, I guess it's writeln

Fixed.

> > LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:112
> > +}, 'document.writeln must throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');
> 
> must not throw and for another document

Fixed.
Comment 10 Ryosuke Niwa 2018-08-07 14:02:49 PDT
Thanks for the review!
Comment 11 Ryosuke Niwa 2018-08-07 14:04:18 PDT
Created attachment 346729 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-08-07 14:05:30 PDT
Comment on attachment 346729 [details]
Patch for landing

Rejecting attachment 346729 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 346729, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/8791153
Comment 13 Ryosuke Niwa 2018-08-07 14:10:10 PDT
Ugh... git is a fucking nightmare.
Comment 14 Ryosuke Niwa 2018-08-07 14:13:09 PDT
Committed r234669: <https://trac.webkit.org/changeset/234669>
Comment 15 Timothy Gu 2018-08-07 14:16:52 PDT
Is it just me or does https://trac.webkit.org/changeset/234669/webkit not contain any changes to Source/WebCore/ besides the ChangeLog?
Comment 16 Ryosuke Niwa 2018-08-07 14:18:38 PDT
No, you're right :( Sigh... WTF.
Comment 17 Timothy Gu 2018-08-07 14:19:46 PDT
Comment on attachment 346729 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=346729&action=review

> Source/WebCore/dom/Document.cpp:2615
>      // FIXME: This should also throw if "document's throw-on-dynamic-markup-insertion counter is greater than 0".

This FIXME should be removed also. Ditto elsewhere.
Comment 18 Ryosuke Niwa 2018-08-07 14:20:03 PDT
Git needs to burn in hell.
Comment 19 WebKit Commit Bot 2018-08-07 14:20:56 PDT
Re-opened since this is blocked by bug 188390
Comment 20 Timothy Gu 2018-08-07 14:21:07 PDT
Comment on attachment 346729 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=346729&action=review

>> Source/WebCore/dom/Document.cpp:2615
>>      // FIXME: This should also throw if "document's throw-on-dynamic-markup-insertion counter is greater than 0".
> 
> This FIXME should be removed also. Ditto elsewhere.

This FIXME should be removed also. Ditto elsewhere.
Comment 21 Timothy Gu 2018-08-07 14:21:39 PDT
Oops double commenting… First time using Bugzilla's Review.
Comment 22 Ryosuke Niwa 2018-08-07 14:47:26 PDT
Created attachment 346735 [details]
Patch for landing
Comment 23 Ryosuke Niwa 2018-08-07 14:47:39 PDT
Comment on attachment 346735 [details]
Patch for landing

Wait for EWS.
Comment 24 Ryosuke Niwa 2018-08-07 16:54:28 PDT
Committed r234680: <https://trac.webkit.org/changeset/234680>
Comment 25 Frédéric Wang (:fredw) 2018-08-08 00:29:18 PDT
Comment on attachment 346687 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=346687&action=review

>>> LayoutTests/ChangeLog:10
>>> +        doesn't test nearly as many edge cases.
>> 
>> I think it would be nice to export the new tests to upstream WPT (nice set of tests BTW!)
> 
> Fixed.

What do you mean by 'Fixed'? I can't find any PR to export tests to the WPT repo on your GitHub activity nor any change in the git history.

>>> LayoutTests/fast/custom-elements/throw-on-dynamic-markup-insertion-counter-reactions.html:61
>>> +}, 'document.open("about:blank") must NOT throw an InvalidStateError when processing custom element reactions for a synchronous constructed custom element');
>> 
>> I don't understand that test without investigating more the spec. Can you please explain?
> 
> Oh, oops, this should read as "document.open(URL)" since I'm not using about:blank anymore. This is a version of document.open which behaves like window.open:
> https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-open-window
> 
> Added a comment above this test case.

Ah OK I was confused by the about:blank. Makes sense.
Comment 26 Ryosuke Niwa 2018-08-08 12:49:26 PDT
(In reply to Frédéric Wang (:fredw) from comment #25)
> Comment on attachment 346687 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346687&action=review
> 
> >>> LayoutTests/ChangeLog:10
> >>> +        doesn't test nearly as many edge cases.
> >> 
> >> I think it would be nice to export the new tests to upstream WPT (nice set of tests BTW!)
> > 
> > Fixed.
> 
> What do you mean by 'Fixed'? I can't find any PR to export tests to the WPT
> repo on your GitHub activity nor any change in the git history.

Oh, that was meant for something else. Feel free to create a WPT PR. I'm not going to get to it anytime soon.
Comment 27 darshan 2018-08-13 09:14:24 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/12447