[webkit-reviews] review granted: [Bug 187319] document.open and document.write must throw while the HTML parser is synchronously constructing a custom element : [Attachment 346687] Fixes the bug

Tue Aug 7 01:24:37 PDT 2018

Frédéric Wang (:fredw) <fred.wang at free.fr> has granted Ryosuke Niwa
<rniwa at webkit.org>'s request for review:
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

Attachment 346687: Fixes the bug

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

--- Comment #8 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 346687
  --> https://bugs.webkit.org/attachment.cgi?id=346687
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-cons
truct.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-cons
truct.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-reac
tions.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-reac
tions.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-reac
tions.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-reac
tions.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-reac
tions.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-reac
tions.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