187319 – document.open and document.write must throw while the HTML parser is synchronously constructing a custom element

RESOLVED FIXED Bug 187319

document.open and document.write must throw while the HTML parser is synchronously constructing a custom element

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

Summary document.open and document.write must throw while the HTML parser is synchron...

Frédéric Wang (:fredw)

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Timothy Gu

Comment 1 2018-07-17 12:22:32 PDT

WPT is available after https://github.com/web-platform-tests/wpt/pull/12037.

Frédéric Wang (:fredw)

Comment 2 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.

Radar WebKit Bug Importer

Comment 3 2018-08-01 22:42:08 PDT

<rdar://problem/42843012>

Ryosuke Niwa

Comment 4 2018-08-06 20:08:33 PDT

Also see https://github.com/whatwg/html/pull/1673

Ryosuke Niwa

Comment 5 2018-08-06 22:38:43 PDT

Created attachment 346686 [details] Fixes the bug

EWS Watchlist

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

Ryosuke Niwa

Comment 7 2018-08-06 22:41:25 PDT

Created attachment 346687 [details] Fixes the bug

Frédéric Wang (:fredw)

Comment 8 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('some text')); 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('some text')); 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

Ryosuke Niwa

Comment 9 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('some text')); > > 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('some text')); > > 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.

Ryosuke Niwa

Comment 10 2018-08-07 14:02:49 PDT

Thanks for the review!

Ryosuke Niwa

Comment 11 2018-08-07 14:04:18 PDT

Created attachment 346729 [details] Patch for landing

WebKit Commit Bot

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

Ryosuke Niwa

Comment 13 2018-08-07 14:10:10 PDT

Ugh... git is a fucking nightmare.

Ryosuke Niwa

Comment 14 2018-08-07 14:13:09 PDT

Committed r234669: <https://trac.webkit.org/changeset/234669>

Timothy Gu

Comment 15 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?

Ryosuke Niwa

Comment 16 2018-08-07 14:18:38 PDT

No, you're right :( Sigh... WTF.

Timothy Gu

Comment 17 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.

Ryosuke Niwa

Comment 18 2018-08-07 14:20:03 PDT

Git needs to burn in hell.

WebKit Commit Bot

Comment 19 2018-08-07 14:20:56 PDT

Re-opened since this is blocked by bug 188390

Timothy Gu

Comment 20 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.

Timothy Gu

Comment 21 2018-08-07 14:21:39 PDT

Oops double commenting… First time using Bugzilla's Review.

Ryosuke Niwa

Comment 22 2018-08-07 14:47:26 PDT

Created attachment 346735 [details] Patch for landing

Ryosuke Niwa

Comment 23 2018-08-07 14:47:39 PDT

Comment on attachment 346735 [details] Patch for landing Wait for EWS.

Ryosuke Niwa

Comment 24 2018-08-07 16:54:28 PDT

Committed r234680: <https://trac.webkit.org/changeset/234680>

Frédéric Wang (:fredw)

Comment 25 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.

Ryosuke Niwa

Comment 26 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.

darshan

Comment 27 2018-08-13 09:14:24 PDT

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/12447

Note You need to log in before you can comment on or make changes to this bug.