Run Microtasks before dispatching onload

Run Microtasks before dispatching onload

Element.registerElement runs "element upgrade algorithm" and expects to resolve unknown tags in "upgrade candidates map" synchronously.
http://w3c.github.io/webcomponents/spec/custom/index.html#appendix-a

However, in current Blink, adding a new unknown element to "upgrade candidates map" is done asynchronously by scheduling a CustomElementMicrotaskResolutionStep to microtask queue. This means that Element.registerElement called from “onload” event handler may not resolve the unknown tags as they are not yet in the “upgrade candidates map”.

This CL workaround the issue by explicitly emptying the microtask queue before running onload handler.

http/tests/inspector/network/network-initiator.html is rebaselined.
- The image resource load is scheduled on a microtask. (See r173943)
- This CL introduces additional microtask checkpoint at <iframe> load.

TEST=LayoutTests/imported/web-platform-tests/custom-elements, LayoutTests/svg/dom/custom-elements.html
BUG=421300, 425790
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187949

[kouhei (in TOK), 2014-12-03 01:42:36]

kouhei@chromium.org changed reviewers:
+ haraken@chromium.org, hayato@chromium.org

[kouhei (in TOK), 2014-12-03 01:42:36]

Would you take a look?

[kouhei (in TOK), 2014-12-04 08:01:14]

friendly ping

[hayato, 2014-12-04 08:11:54]

I'm afraid that I am not a good reviewer for this. I don't have any insight why
this resolves the issue. :(

[kouhei (in TOK), 2014-12-05 04:00:46]

kouhei@chromium.org changed reviewers:
+ morrita@chromium.org

[kouhei (in TOK), 2014-12-05 04:00:47]

morrita: Sorry to bother you. Would you mind taking a look?

[Hajime Morrita, 2014-12-05 18:29:56]

morrita@chromium.org changed reviewers:
+ arv@chromium.org, dominicc@chromium.org

[Hajime Morrita, 2014-12-05 18:29:56]

What I was told is that adding random microtask invocation generally isn't a
good idea. I'd rather do this just before the script invocation part if
possible. See HTMLScriptRunner for example.

[kouhei (in TOK), 2014-12-08 16:29:47]

kouhei@chromium.org changed reviewers:
+ skyostil@chromium.org

[dominicc (has gone to gerrit), 2014-12-09 02:19:21]

The HTML spec or maybe DOM spec should be definitive on whether we *must* run a
microtask here (usually it's called "stable state" IIRC.) Should we?

If not, it is the tests that are buggy and should be de-bugged.

In the past there have been spec requests for a "Custom Elements ready" event,
dglazkov will know the context better than me.

Now that we are in a Promises world, it would be nice if elements had a Promise
for resolution, but again that is a spec thing.

[kouhei (in TOK), 2014-12-15 05:56:50]

On 2014/12/09 02:19:21, dominicc wrote:
> The HTML spec or maybe DOM spec should be definitive on whether we *must* run
a
> microtask here (usually it's called "stable state" IIRC.) Should we?

The spec does not say that we *must* run a microtask here, but it also doesn't
say that we shouldn't.

> If not, it is the tests that are buggy and should be de-bugged.

It looks like the web-platform-tests also depend on that Microtask queue is
emptied when onload is called:
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

I'm planning to change how HTMLDocumentParser is scheduled in
https://codereview.chromium.org/673603002/ .
Before the CL, the microtasks were run at end of random task at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
However, after the CL, we might not get such task to run microtask hook if we
are unlucky.

[kouhei (in TOK), 2014-12-15 09:02:39]

Updated description and uploaded new patch.

I talked offline with dominicc, and we found that the issue was that adding a
new unknown element to "upgrade candidates map" is done asynchronously in
current Blink, and registerElement may not find the newly added unknown element
if microtasks hadn't run.
PS3 workarounds the issue by explicitly clearing out custom element task queue
before onload event handlers.

> The HTML spec or maybe DOM spec should be definitive on whether we *must* run
a
> microtask here (usually it's called "stable state" IIRC.) Should we?
The HTML spec for parsing end is
https://html.spec.whatwg.org/multipage/syntax.html#stop-parsing
Blink follows the spec for the most part, but Blink doesn't follow the spec on
4-5, but fires DOMContentLoaded task synchronously. ( http://crbug.com/425790 ) 
As spinning event loop also includes emptying the microtask queue, I think we
can run the microtask as in PS2.

morrita, dominicc: wdyt?

[kouhei (in TOK), 2014-12-17 09:26:26]

friendly ping :)

I just re-read http://crbug.com/425790 and realized that the LayoutTests are
hitting the exact case mentioned in the bug.

As this is blocking Blink Scheduler, I'd like to resolve the issue at least
enough to pass the LayoutTests.

Given that complete fix https://codereview.chromium.org/672843002/ is
controversial, I'd like to try landing this partial fix.

> On 2014/12/09 02:19:21, dominicc wrote:
> > The HTML spec or maybe DOM spec should be definitive on whether we *must*
run
a
> > microtask here (usually it's called "stable state" IIRC.) Should we?
> The spec does not say that we *must* run a microtask here, but it also doesn't
> say that we shouldn't.
Sorry I was wrong here. Spec do say we *must* run a microtask here.

HTML spec says we should "queue a task" to fire DOMContentLoaded task,
and spin the event loop. Spinning the event loop includes running microtasks.

However, we currently fire DOMContentLoaded event synchronously.
This is a bug, but it is hard to land the complete fix immediately, as it breaks
js-test.js harness, which sadly depends on DOMContentLoaded being synchronously
fired.

I think PS4 is a reasonable step forward to take at the moment. Would you take a
look?

[dominicc (has gone to gerrit), 2014-12-17 14:30:50]

One comment inline.

I have a question about this--why did this work in the past, and what broke it?

Some notes about our discussion, to aid my memory:

Custom Elements spec is precise about when its callbacks run; they almost appear
synchronous to the script author by running them at least before
entering/returning to script. To make Imports run their Custom Element callbacks
in order, the resolution step became asynchronous but this was not observable
because of the way Imports blocked onload, etc. Now something changed in the
parser than upset that precarious balance and we are trying to fix that.

[dominicc (has gone to gerrit), 2014-12-17 14:31:03]

https://codereview.chromium.org/774033002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/774033002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4631: // Empty microtask queue to ensure custom
element is resolved before onload event.
I would remove this and put a FIXME with a link to the HTML spec that says to
spin the event loop before dispatching DOMContentLoaded and note that this is
simulating that.

[kouhei (in TOK), 2014-12-18 05:33:55]

> I have a question about this--why did this work in the past,
This *worked* in the past by coincidence.
With the old scheduler and the old HTMLParser, we got a lucky task scheduling
99% of the time
that HTMLParser constructed the DOM asynchronously using multiple tasks.
If we put a dummy delay (e.g. usleep(1000)) in between random places, the
current logic would still fail.

> … and what broke it?
Enabling Blink Scheduler, and moving HTML chunk processing task to right
priority CL
(https://codereview.chromium.org/673603002/) greatly decreases the chance that
HTMLParser yield during tree construction.
For example, before https://codereview.chromium.org/673603002/ CL,
parser processed each chunk in IPC event handler event when it was not blocked.
After the CL, HTMLParser would just queue the received chunks in IPC handler,
and defers the processing to separate task. For simple pages like seen in
LayoutTests,
the chances are we process all the queued chunks in a single pump task, meaning
we
don’t get a chance to spin a event loop and thus no microtask checkpoint.

https://codereview.chromium.org/774033002/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/774033002/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4631: // Empty microtask queue to ensure custom
element is resolved before onload event.
On 2014/12/17 14:31:03, dominicc wrote:
> I would remove this and put a FIXME with a link to the HTML spec that says to
> spin the event loop before dispatching DOMContentLoaded and note that this is
> simulating that.

Done.

[kouhei (in TOK), 2015-01-05 02:57:27]

friendly ping

[dominicc (has gone to gerrit), 2015-01-05 07:28:54]

On 2015/01/05 at 02:57:27, kouhei wrote:
> friendly ping

LGTM, modulo tightening the comment up a bit and maybe moving that protect(this)
if you think we need to. I have a suggestion inline for comment wording but you
can probably improve on it.

[dominicc (has gone to gerrit), 2015-01-05 07:29:39]

(Actually publish the comments.)

https://codereview.chromium.org/774033002/diff/80001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/774033002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4621: // FIXME: DOMContentLoaded should be
dispatched in a queued task,
I think a good FIXME does three things:

1. Keeps it short.
2. Explains what needs to be fixed.
3. Explains when the FIXME can be addressed.

I think you don't need the comment here, because it's not a FIXME as much as an
actual bug, so you could comment on Issue 425790 directly. The spec links are
probably useful in the bug thread?

Below I think you want something like:

// Ensure Custom Element callbacks are drained before DOMContentLoaded.
// FIXME: Remove this ad-hoc checkpoint when DOMContentLoaded is dispatched in a
queued task, which will do a checkpoint anyway. (Issue 425790)

https://codereview.chromium.org/774033002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4643: RefPtrWillBeRawPtr<Document> protect(this);
Why doesn't this protect need to be moved to before
Microtask::performCheckpoint? Can't the checkpoint also invoke script?

[kouhei (in TOK), 2015-01-06 01:37:06]

Thanks!

Shortened FIXME comment per suggestion.

https://codereview.chromium.org/774033002/diff/80001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/774033002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4621: // FIXME: DOMContentLoaded should be
dispatched in a queued task,
On 2015/01/05 07:29:39, dominicc wrote:
> I think a good FIXME does three things:
> 
> 1. Keeps it short.
> 2. Explains what needs to be fixed.
> 3. Explains when the FIXME can be addressed.
> 
> I think you don't need the comment here, because it's not a FIXME as much as
an
> actual bug, so you could comment on Issue 425790 directly. The spec links are
> probably useful in the bug thread?
> 
> Below I think you want something like:
> 
> // Ensure Custom Element callbacks are drained before DOMContentLoaded.
> // FIXME: Remove this ad-hoc checkpoint when DOMContentLoaded is dispatched in
a
> queued task, which will do a checkpoint anyway. (Issue 425790)

Done.

https://codereview.chromium.org/774033002/diff/80001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4643: RefPtrWillBeRawPtr<Document> protect(this);
On 2015/01/05 07:29:39, dominicc wrote:
> Why doesn't this protect need to be moved to before
> Microtask::performCheckpoint? Can't the checkpoint also invoke script?

Done.

[kouhei (in TOK), 2015-01-06 01:37:11]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-01-06 01:38:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/774033002/100001

[commit-bot: I haz the power, 2015-01-06 03:00:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-06 03:00:20]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[kouhei (in TOK), 2015-01-07 01:24:36]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2015-01-07 01:25:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/774033002/120001

[commit-bot: I haz the power, 2015-01-07 05:26:12]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187949

[adamk, 2015-01-07 16:50:31]

adamk@chromium.org changed reviewers:
+ adamk@chromium.org

[adamk, 2015-01-07 16:50:32]

Sorry for the late comment, but I just saw this go by: why is there no test
included in this patch (for example, the test case attached to bug 425790)?

[kouhei (in TOK), 2015-01-09 04:58:27]

On 2015/01/07 16:50:32, adamk wrote:
> Sorry for the late comment, but I just saw this go by: why is there no test
> included in this patch (for example, the test case attached to bug 425790)?
Yes, this should have a test.
Added missing test in CL: https://codereview.chromium.org/774033002/

[blink-reviews, 2015-01-09 05:08:54]

Which CL? It points to this CL.

I thought that there were existing tests for this; that this was unbreaking
something?

On Fri, Jan 9, 2015 at 1:58 PM, <kouhei@chromium.org> wrote:

> On 2015/01/07 16:50:32, adamk wrote:
>
>> Sorry for the late comment, but I just saw this go by: why is there no
>> test
>> included in this patch (for example, the test case attached to bug
>> 425790)?
>>
> Yes, this should have a test.
> Added missing test in CL: https://codereview.chromium.org/774033002/
>
> https://codereview.chromium.org/774033002/
>

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

[kouhei (in TOK), 2015-01-09 05:14:55]

On 2015/01/09 05:08:54, blink-reviews wrote:
> Which CL? It points to this CL.
> 
> I thought that there were existing tests for this; that this was unbreaking
> something?

Sorry, the correct URL is https://codereview.chromium.org/839673004/

The existing tests rely on this event invocation order, but there were no test
which explicitly check the ordering.