Perform a microtask checkpoint for document.write('<script><\/script>')

Perform a microtask checkpoint for document.write('<script><\/script>')

http://whatwg.org/html#scriptEndTag

The spec does 'Perform a microtask checkpoint' unconditionally for 'An
end tag whose tag name is "script"', which means that document.write
can trigger it while a script is running.

A spec bug was filed for the similar situation with 'Provide a stable
state' on the assumption that this was a mistake, but it was not:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24361

Unless performing a microtask checkpoint here will lead to other
complications, it's simpler to just align with the spec.

BUG=340322
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166445

[philipj_slow, 2014-02-03 18:29:57]

PTAL, this is something I noticed while trying to add "provide a stable state"
around here, something that's needed to align some HTMLMediaElement algorithms
with the spec.

[philipj_slow, 2014-02-05 03:17:36]

tkent, can you PTAL? The original reviewers have reviewed around this code
before, but are far from my timezone...

[tkent, 2014-02-05 03:25:51]

rubber-stamping lgtm

I don't understand this code change well. Anyway, the CL description sounds
reasonable.

[philipj_slow, 2014-02-05 03:31:38]

On 2014/02/05 03:25:51, tkent wrote:
> rubber-stamping lgtm
> 
> I don't understand this code change well. Anyway, the CL description sounds
> reasonable.

Thanks! I'll commit this now and send a separate email to esprehn to take a look
after the fact, since he authored the FIXME that I'm removing.

[philipj_slow, 2014-02-05 03:34:48]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-02-05 03:41:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/130983011/1

[commit-bot: I haz the power, 2014-02-05 03:48:18]

Change committed as 166445

[esprehn, 2014-02-11 00:44:49]

I don't think this patch is right, now appendChild(script) will cause a
microtask checkpoint in the middle of arbitrary script.

I think we should just change the spec here, you shouldn't be able to trigger a
checkpoint while script is still running.

adamk@ Does it seem right that doing appendChild(script) will force a delivery
of all MutationRecords after this patch?

https://codereview.chromium.org/130983011/diff/1/Source/core/html/parser/HTML...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/130983011/diff/1/Source/core/html/parser/HTML...
Source/core/html/parser/HTMLScriptRunner.cpp:289:
Microtask::performCheckpoint();
This is not correct, now appendChild(script) does a micro task checkpoint right
then.

[adamk, 2014-02-11 01:04:11]

On 2014/02/11 00:44:49, esprehn wrote:
> I don't think this patch is right, now appendChild(script) will cause a
> microtask checkpoint in the middle of arbitrary script.
> 
> I think we should just change the spec here, you shouldn't be able to trigger
a
> checkpoint while script is still running.
> 
> adamk@ Does it seem right that doing appendChild(script) will force a delivery
> of all MutationRecords after this patch?

Agreed that appendChild should definitely not synchronously perform a microtask
checkpoint.

As for whether document.write('<script></script>') needs to avoid triggering a
checkpoint, I'm not sure of the easiest spec fix for that, but this isn't the
right forum to discuss it...

[eseidel, 2014-02-11 01:33:26]

I don't really understand "microtask checkpoints", but it looks like the spec is
trying to identify times when script execution is OK?

It seems </script> wants to do this just before script execution, but probably
not during nested script execution?

If you're going to do this, you should write a test which uses mutation events
to call document.close() or document.write() and test what the expected parse
output its.  document.write handling is super delicate, and I doubt you want to
change it this way.

[esprehn, 2014-02-11 01:34:59]

Short term I think we should roll this patch out, it makes mutation observers
deliver records at times which are not correct.

[philipj_slow, 2014-02-11 03:42:43]

Bah, I should have tested appendChild(script), but it looks like that much
works:
https://codereview.chromium.org/146043008/

I'll also add tests for document.close() and document.write() to that CL.

BTW, I'd be happy if the spec just didn't provide a stable state or a microdata
checkpoint on nested script execution, but as you can tell from the spec bug
mentioned, Hixie disagreed. Feel free to give it another try :)

[philipj_slow, 2014-02-11 04:03:09]

I reopened https://www.w3.org/Bugs/Public/show_bug.cgi?id=24361

Please weigh in there if you care about what the spec says.

[philipj_slow, 2014-02-11 17:26:13]

I'm going to be gone for a few days, so feel free to revert this if my follow-up
does address the concerns or if you think the spec needs to change anyway.

[rafaelw, 2014-02-11 19:19:25]

Yeah, this is definitely wrong. I'm not sure what the right spec fix is, but we
can't run a microtask checkpoint with page script on the stack. Tkent, can you
please roll this out?

[tkent, 2014-02-12 03:50:36]

A revert of this CL has been created in
https://codereview.chromium.org/153913010/ by tkent@chromium.org.

The reason for reverting is: Broke MutationObserver consistency.
.

[tkent, 2014-02-12 03:58:25]

On 2014/02/12 03:50:36, tkent wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/153913010/ by mailto:tkent@chromium.org.
> 
> The reason for reverting is: Broke MutationObserver consistency.
> .

Oh, I reverted this before finishing reading all of comments.  philipj	 said
this didn't break MutaitonObserver. I'll revert the revert.

[rafaelw, 2014-02-13 19:07:37]

ok. now, I'm lost in reverts and I can't tell what the current semantics are.

Here's what we *need*:

-Layout tests asserting that *neither* document.write('<script></script>') NOR
document.appendChild(scripeElement) triggers delivery to mutation observers.

To be more precise: We cannot break the following invariant:

Mutation Observer callbacks ALWAYS run on a fresh script stack (no other page
script can be running when a mutation observer callback fires).

tkent & phillipj, can you please ensure this is the case.

[philipj_slow, 2014-02-13 23:38:31]

On 2014/02/13 19:07:37, rafaelw wrote:
> ok. now, I'm lost in reverts and I can't tell what the current semantics are.
> 
> Here's what we *need*:
> 
> -Layout tests asserting that *neither* document.write('<script></script>') NOR
> document.appendChild(scripeElement) triggers delivery to mutation observers.
> 
> To be more precise: We cannot break the following invariant:
> 
> Mutation Observer callbacks ALWAYS run on a fresh script stack (no other page
> script can be running when a mutation observer callback fires).
> 
> tkent & phillipj, can you please ensure this is the case.

I'm in Singapore on vacation now, but I can make it so on Monday. If the branch
point is before that I'm going to have to entrust the reverting to tkent. In the
meantime, it would be great if you could explain in the spec bug *why* microtask
checkpoints checkpoints mustn't be run with scripts on the stack, so that the
spec will actually come to agree with you. (In the case of a stable state, it's
silly but seemingly harmless to provide a stable state in document.write,
because synchronous sections never modify the DOM.)

[rafaelw1, 2014-02-14 18:33:33]

On Thu, Feb 13, 2014 at 3:38 PM, <philipj@opera.com> wrote:

> On 2014/02/13 19:07:37, rafaelw wrote:
>
>> ok. now, I'm lost in reverts and I can't tell what the current semantics
>> are.
>>
>
>  Here's what we *need*:
>>
>
>  -Layout tests asserting that *neither* document.write('<script></script>')
>> NOR
>> document.appendChild(scripeElement) triggers delivery to mutation
>> observers.
>>
>
>  To be more precise: We cannot break the following invariant:
>>
>
>  Mutation Observer callbacks ALWAYS run on a fresh script stack (no other
>> page
>> script can be running when a mutation observer callback fires).
>>
>
>  tkent & phillipj, can you please ensure this is the case.
>>
>
> I'm in Singapore on vacation now, but I can make it so on Monday. If the
> branch
>

The M34 branch point *is* Monday, but it's relatively easy to get patches
applied to the branch so early in the cycle. I leave it to you two to
decide.


> point is before that I'm going to have to entrust the reverting to tkent.
> In the
> meantime, it would be great if you could explain in the spec bug *why*
> microtask
> checkpoints checkpoints mustn't be run with scripts on the stack, so that
> the
> spec will actually come to agree with you. (In the case of a stable state,
> it's
> silly but seemingly harmless to provide a stable state in document.write,
> because synchronous sections never modify the DOM.)
>

Done: https://www.w3.org/Bugs/Public/show_bug.cgi?id=24361#c10


>
> https://codereview.chromium.org/130983011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[philipj_slow, 2014-02-17 05:35:17]

Here's the review that basically reverts things to the way they were:
https://codereview.chromium.org/169093004/