Summary: | document.open and document.write must throw while the HTML parser is synchronously constructing a custom element | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||||||
Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, dbates, dkadu, esprehn+autocc, ews-watchlist, gyuyoung.kim, kangil.han, rniwa, rwlbuis, timothygu99, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
URL: | https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#throw-on-dynamic-markup-insertion-counter | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=187802 https://bugs.webkit.org/show_bug.cgi?id=187907 |
||||||||||||
Bug Depends on: | 188336, 188390 | ||||||||||||
Bug Blocks: | 154907 | ||||||||||||
Attachments: |
|
Description
Frédéric Wang (:fredw)
2018-07-04 02:43:06 PDT
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. 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 |