"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.
WPT is available after https://github.com/web-platform-tests/wpt/pull/12037.
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.
<rdar://problem/42843012>
Also see https://github.com/whatwg/html/pull/1673
Created attachment 346686 [details] Fixes the bug
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.
Created attachment 346687 [details] Fixes the bug
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
(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.
Thanks for the review!
Created attachment 346729 [details] Patch for landing
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
Ugh... git is a fucking nightmare.
Committed r234669: <https://trac.webkit.org/changeset/234669>
Is it just me or does https://trac.webkit.org/changeset/234669/webkit not contain any changes to Source/WebCore/ besides the ChangeLog?
No, you're right :( Sigh... WTF.
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.
Git needs to burn in hell.
Re-opened since this is blocked by bug 188390
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.
Oops double commenting… First time using Bugzilla's Review.
Created attachment 346735 [details] Patch for landing
Comment on attachment 346735 [details] Patch for landing Wait for EWS.
Committed r234680: <https://trac.webkit.org/changeset/234680>
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.
(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.
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/12447