Last Comment Bug 789315 - MutationObservers are notified about parsed content after <script> runs, not before
: MutationObservers are notified about parsed content after <script> runs, not ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla45
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
: 1180927 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-06 15:51 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2015-11-27 07:26 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
simple testcase (583 bytes, text/html)
2012-09-06 15:51 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details
WIP (15.54 KB, patch)
2012-09-21 15:41 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
v1 (3.13 KB, patch)
2015-10-05 12:34 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
khuey: review+
amarchesini: feedback+
Details | Diff | Splinter Review
-extra space (3.13 KB, patch)
2015-11-26 09:28 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
fix for a buggy wpt test (1.81 KB, patch)
2015-11-26 11:44 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
josh: review+
Details | Diff | Splinter Review
remove wpt .ini now that we pass the tests (3.63 KB, patch)
2015-11-26 11:44 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
fix for a buggy wpt test (1.81 KB, patch)
2015-11-26 12:34 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-09-06 15:51:23 PDT
Created attachment 659028 [details]
simple testcase

When parsing and we hit a <script> element, we should flush all MutationObserver notifications before executing the contents of the script.

Note that we should do this after inserting the <script> though. I.e. the MutationObservers should have been notified about the <script> element before the contents of the element begins to run.
Comment 1 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-07 00:47:58 PDT
(In reply to Jonas Sicking (:sicking) from comment #0)
> When parsing and we hit a <script> element, we should flush all
> MutationObserver notifications before executing the contents of the script.
Why?
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-07 00:49:58 PDT
Looks like spec's event loop handling is rather odd, but ok, we can do this.
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-07 00:58:32 PDT
Er, we're doing the right thing per spec, so the spec needs to be changed too.
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-07 00:59:55 PDT
(Spec just requires stable state, but that doesn't mean end of microtask or end of task,
nor is there any special case for mutation observers.)
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-09-10 04:49:07 PDT
Yeah, sorry, I'm not sure what the spec requires here. Technically we definitely don't follow the spec right now since the spec calls for no notifications to be fired due to parsing.

It simply seemed to me that the behavior in comment #0 made the most sense.

My thinking was that running a script is very similar to a microtask. And generally when the outermost microtask starts we have flushed all mutationobserver notifications. The spec defines that notifications are flushed at the end of each outermost microtask, but the effect is that they are flushed by the time the next microtask starts.

The advantage over current behavior is script can count on application logic being in a consistent state by the time the script runs. I.e. if you have a MutationObserver which implements a template library, you can know that all the template logic has been updated to the page's current state by the time the script runs.

Right now you can't really know which part of the DOM has been notified about since that depends on when the parser last decided to return to the event loop.

But I'm very open to other proposals. I mostly filed this because that's how I had assumed it would work. No deeper thinking behind it :)
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-21 15:41:55 PDT
Created attachment 663583 [details] [diff] [review]
WIP

Not tested properly yet.
HTML5 parsing part should be ok (it is actually very tiny change with diff -w),
but I need to think about the old content sink stuff still, and actually test them.
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-09-21 15:46:09 PDT
(and looks like the xmlcontensink stuff is wrong)
Comment 8 Hixie (not reading bugmail) 2012-10-23 15:24:24 PDT
Spec fixed.
Comment 9 mjh563 2012-11-07 17:24:02 PST
I've noticed that you can get the requested behaviour if you add a 'beforescriptexecute' handler. For example, if I add this line to the testcase:

window.addEventListener("beforescriptexecute", function(){console.log("At beforescriptexecute")}, false);

I get the output:

[01:22:09.334] At beforescriptexecute
[01:22:09.335] added #text
[01:22:09.335] added B
[01:22:09.335] added #text
[01:22:09.335] added SCRIPT
[01:22:09.335] I'm now in script tag
[01:22:09.336] At beforescriptexecute
[01:22:09.336] added #text
[01:22:09.336] added A
[01:22:09.337] added #text
[01:22:09.337] added SCRIPT
[01:22:09.338] I'm now in second script tag
[01:22:09.338] added #text
[01:22:09.339] added DIV
[01:22:09.339] added #text

Can that be relied on as a workaround?
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-07 17:27:48 PST
Yes. If there are listeners for beforescriptexecute, the MutationObserver callbacks will be
called at the end of microtask, which is end of event listener.

But I'll try to get back to this bug ASAP, and then we'll need to change the specs a bit ...
Comment 11 Olli Pettay [:smaug] (vacation Aug 25-28) 2015-10-05 11:54:02 PDT
*** Bug 1180927 has been marked as a duplicate of this bug. ***
Comment 12 Olli Pettay [:smaug] (vacation Aug 25-28) 2015-10-05 11:54:39 PDT
I'll deal with this in a bit different way.
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2015-10-05 12:34:19 PDT
Created attachment 8669869 [details] [diff] [review]
v1

I'm sure khuey likes microtasks these days :)
Anyhow, the spec effectively requires a checkpoint before executing a script.
The spec is just super convoluted.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e94862364160
Comment 14 Andrea Marchesini [:baku] 2015-11-17 12:03:32 PST
Comment on attachment 8669869 [details] [diff] [review]
v1

Review of attachment 8669869 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry but I cannot review this patch. I don't know all the implications of performing a microtask checkpoint.

::: dom/base/test/test_bug789315.html
@@ +25,5 @@
> +              addedNode.mutationObserverHasNotified = true;
> +            }
> +          }
> +        }
> +      

extra spaces.
Comment 15 Olli Pettay [:smaug] (vacation Aug 25-28) 2015-11-26 09:28:45 PST
Created attachment 8692595 [details] [diff] [review]
-extra space
Comment 16 Olli Pettay [:smaug] (vacation Aug 25-28) 2015-11-26 11:44:01 PST
Created attachment 8692627 [details] [diff] [review]
fix for a buggy wpt test
Comment 17 Olli Pettay [:smaug] (vacation Aug 25-28) 2015-11-26 11:44:41 PST
Created attachment 8692628 [details] [diff] [review]
remove wpt .ini now that we pass the tests
Comment 18 Josh Matthews [:jdm] 2015-11-26 12:28:58 PST
Comment on attachment 8692627 [details] [diff] [review]
fix for a buggy wpt test

Review of attachment 8692627 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/dom/nodes/MutationObserver-document.html
@@ +53,5 @@
> +                         },
> +                         target: function () {
> +                          return document.getElementById("n00");
> +                         }},
> +                         {type: "childList",

nit: unindent this by 1 space.
Comment 19 Olli Pettay [:smaug] (vacation Aug 25-28) 2015-11-26 12:34:17 PST
Created attachment 8692640 [details] [diff] [review]
fix for a buggy wpt test

Note You need to log in before you can comment on or make changes to this bug.