|
|
Created:
6 years, 10 months ago by philipj_slow Modified:
6 years, 10 months ago Reviewers:
adamk, tkent, esprehn, rafaelw1, eseidel, rafaelw CC:
blink-reviews, dglazkov+blink, adamk+blink_chromium.org, tonyg, abarth-chromium Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionPerform 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
Patch Set 1 #
Total comments: 1
Created: 6 years, 10 months ago
Messages
Total messages: 21 (0 generated)
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.
tkent, can you PTAL? The original reviewers have reviewed around this code before, but are far from my timezone...
rubber-stamping lgtm I don't understand this code change well. Anyway, the CL description sounds reasonable.
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.
The CQ bit was checked by philipj@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/130983011/1
Message was sent while issue was closed.
Change committed as 166445
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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...
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
Short term I think we should roll this patch out, it makes mutation observers deliver records at times which are not correct.
Message was sent while issue was closed.
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 :)
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
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. .
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.)
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.
Message was sent while issue was closed.
Here's the review that basically reverts things to the way they were: https://codereview.chromium.org/169093004/ |