1081038 - createdCallback is not running during the constructor when run from the scratchpad

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gabor Krizsanits (INACTIVE)]

var XBooPrototype = Object.create(HTMLElement.prototype);
XBooPrototype.createdCallback = function() {
  this.style.fontStyle = 'italic';
}
var XBoo = document.registerElement('x-boo', {
  prototype: XBooPrototype
});
var xboo = new XBoo();

// xboo.style.fontStyle is not set here

Comment 1

[Boris Zbarsky [:bzbarsky]]

Worksforme in a recentish m-c build (rev 9ee9e193fc48) with webcomponents enabled...

Comment 2

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Worksforme in a recentish m-c build (rev 9ee9e193fc48) with webcomponents
> enabled...

That is weird, how did you test it? I must be doing something wrong then, but I've just tried it again in a fresh m-c build and still does not work for me... I tried it from scratchpad and do console.log or throw as well for xboo.style.fontStyle but still does not give me italic... (or anything)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Worksforme in a current trunk nightly too.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

So I tried just pasting my script into scratchpad.

It alerts empty string, but the element in the document has the right style (both style attribute and rendered style).

In fact, if I just inspect xboo.style.fontStyle off a 0-ms timeout, it shows up correctly.

So apparently the createdCallback is not running during the constructor when run from the scratchpad.

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #5)
> So apparently the createdCallback is not running during the constructor when
> run from the scratchpad.

Thanks, that seems to be it. I found this bug during running some polymer tests, so it's not only for scratchpad, but seems like a good starting point.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, so when I call this from an inline script, in nsDocument::EnqueueLifecycleCallback we have lastData == nullptr and nsContentUtils::MicroTaskLevel == 1, so shouldPushElementQueue ends up true.  Then we call nsContentUtils::AddScriptRunner, sScriptBlockerCount is 0, so we run the runnable immediately.  In Run(), mIsBaseQueue is false, so we go ahead and run the callback.

When running from scratchpad, we still have lastData == nullptr, but now nsContentUtils::MicroTaskLevel == 0.  So shouldPushElementQueue ends up false.  Then we skip adding the script runner and afaict just end up sitting there until .... something.

I have no idea why we're examining the microtask level here, nor why the scratchpad is not setting up microtasks correctly.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, and for the script case we enter the microtask in nsJSUtils::EvaluateString when we do "nsAutoMicroTask mt;" before running the contents of the <script>.  Apparently whatever the scratchpad does it doesn't do that...  It seems to go directly through nsXPCWrappedJS and then DebuggerGenericEval to get back to the C++, without ever going through the places where we'd start/stop a microtask.

Comment 9

[William Chen [:wchen]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> I have no idea why we're examining the microtask level here, nor why the
> scratchpad is not setting up microtasks correctly.

We examine the microtask level because the spec defines that the base element queue gets processed at microtask checkpoints (we do this in nsContentUtils::LeaveMicroTask), so we don't want to add another script runner to process the element queue at the top of the stack when it is the base element queue.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So your real issue is that the devtools stuff doesn't do a microtask.  That needs to be fixed.

Incidentally, shouldn't you also do a microtask checkpoint when the event loop spins?  Mutation observers certainly do....

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> So your real issue is that the devtools stuff doesn't do a microtask.  That
> needs to be fixed.

I bet that is evalInSandbox, and it seems like it does not do microtask indeed. Bobby is there a reason for it or is it just a bug and I should add it?

Side note: shouldn't we fix this in workers too? I see JS::Evaluate calls there too without any mt handling... or maybe the JS engine should do the mt handling a bit deeper, so consumers of these calls should not have to be worried about it?

Comment 12

[Bobby Holley (:bholley)]

It seems like AutoEntryScript should probably do a microtask, right?

Comment 13

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #12)
> It seems like AutoEntryScript should probably do a microtask, right?

Sounds like the perfect place to put it indeed, thanks.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Attachment: entering microtask in AutoEntryScript

So entering microtask before evalInSandbox does solve the problem, but it's main thread only, so I have no idea how to deal with microtasks for workers.

I'm not too familiar with this EnterMicroTask API, is there someone who can validate that by doing this we don't accidentally do a microtask checkpoint too early (after each evalInSandbox call for example)?

relevant spec: https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-9

Bobby, I flag you for a feedback for this patch, not a final version, have not added test either, I just need feedback since I've got some doubts... (also let me know if I should ask someone else, not sure who is the expert of microtasks, Olli maybe?)

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8516949 [details] [diff] [review]
entering microtask in AutoEntryScript

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

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #14)
> So entering microtask before evalInSandbox does solve the problem, but it's
> main thread only, so I have no idea how to deal with microtasks for workers.

Microtasks are main-thread-only.
 
> I'm not too familiar with this EnterMicroTask API, is there someone who can
> validate that by doing this we don't accidentally do a microtask checkpoint
> too early (after each evalInSandbox call for example)?

The spec only discusses this in terms of Window, so it's kind of a moot point. IIUC, microtasks run when we finish the outer-most run-to-completion of the innermost event loop. So queuing a microtask is basically like running a script off a special queue at the front of the event loop.

> Bobby, I flag you for a feedback for this patch, not a final version, have
> not added test either, I just need feedback since I've got some doubts...
> (also let me know if I should ask someone else, not sure who is the expert
> of microtasks, Olli maybe?)

Boris and I discussed this and think it makes sense. But yes, Olli should probably review it too.

We should also included a subsequent patch that nukes nsAutoMicroTask, because I don't think we need it anymore.

::: dom/base/ScriptSettings.cpp
@@ +466,5 @@
>  {
>    MOZ_ASSERT(aGlobalObject);
>    MOZ_ASSERT_IF(!aCx, aIsMainThread); // cx is mandatory off-main-thread.
>    MOZ_ASSERT_IF(aCx && aIsMainThread, aCx == FindJSContext(aGlobalObject));
> +  if (NS_IsMainThread()) {

You should be using aIsMainThread here to avoid checking TLS.

@@ +477,5 @@
>    // GC when we pop a script entry point. This is a useful heuristic that helps
>    // us out on certain (flawed) benchmarks like sunspider, because it lets us
>    // avoid GCing during the timing loop.
>    JS_MaybeGC(cx());
> +  if (NS_IsMainThread()) {

We should stash aIsMainThread in a member and use that here.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Bobby Holley (:bholley) from comment #15)
> 
> Microtasks are main-thread-only.
In Gecko, but if say Promises start to use microtasks, they should use microtasks also in workers.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Promises don't use microtasks.

Comment 18

[Gabor Krizsanits (INACTIVE)]

Sigh. Still, cannot find a way to reproduce this issue in our automated testing framework...

Comment 19

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=af68586aef1a

Comment 20

[Gabor Krizsanits (INACTIVE)]

Attachment: microtask in AutoEntryScript. v1

Sorry for the lack of test, but none of the tests I created reproduce the issue, what I experienced in the scratchpad.

Comment 21

[Bobby Holley (:bholley)]

Comment on attachment 8520774 [details] [diff] [review]
microtask in AutoEntryScript. v1

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

Looks good, but can you add another patch that rips out nsAutoMicroTask from the places where we already have AutoEntryScript, per comment 15? Followup bug is fine, but I want it to happen now.

::: dom/base/ScriptSettings.cpp
@@ +477,5 @@
>  {
>    // GC when we pop a script entry point. This is a useful heuristic that helps
>    // us out on certain (flawed) benchmarks like sunspider, because it lets us
>    // avoid GCing during the timing loop.
>    JS_MaybeGC(cx());

On a very fuzzy intuitive level, I think it _might_ be better to GC after leaving the microtask, since leaving the microtask _might_ cause us do more manipulation of the same JS heap data that was being manipulated by the script proper. So maybe reverse the order here?

Comment 22

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #21)
> Looks good, but can you add another patch that rips out nsAutoMicroTask from
> the places where we already have AutoEntryScript, per comment 15? Followup
> bug is fine, but I want it to happen now.

Doing that part here makes sense, but if you want to get rid of nsAutoMicroTask completely as you say in Comment 15, that should go into a separate bug, because these places do not use AutoEntryScript.

/dom/geolocation/nsGeolocation.cpp
    line 321 -- nsAutoMicroTask mt;
    line 623 -- nsAutoMicroTask mt;

/layout/base/nsPresContext.cpp
    line 1942 -- nsAutoMicroTask mt;

/layout/base/nsRefreshDriver.cpp
    line 1206 -- nsAutoMicroTask mt;

Comment 23

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: Removing nsAutoMicroTask where we have AutoEntryScript. v1

Comment 24

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d3ab34903a79

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8521202 [details] [diff] [review]
part2: Removing nsAutoMicroTask where we have AutoEntryScript. v1

Can we not also get rid of the nsContentUtils::Enter/LeaveMicroTask in CallbackObject::CallSetup?

Comment 26

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: Removing nsAutoMicroTask where we have AutoEntryScript. v2

Removed from CallbackObject::CallSetup too.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=79da093bd7f8

Comment 27

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f8365b8a92
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9243c59c3e56

Comment 28

[Carsten Book [:Tomcat]]

sorry had to back out this changes for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3848635&repo=mozilla-inbound

Comment 29

[Gabor Krizsanits (INACTIVE)]

(In reply to Carsten Book [:Tomcat] from comment #28)
> sorry had to back out this changes for test failures like
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3848635&repo=mozilla-inbound

It's indeed caused by the first patch. I couldn't catch this with my try pushed because the only fails after a quite recent change:

http://hg.mozilla.org/mozilla-central/diff/779e64334c1d/browser/devtools/inspector/test/browser_inspector_delete-selected-node-03.js

It seems like we execute some microtask earlier than before because of the patch and because of that some promises are resolved too early... Not sure yet, I'm looking into this.

Comment 30

[Gabor Krizsanits (INACTIVE)]

Attachment: part0: fixing up inspector test

It was just a lucky coincident it seems that this test was not an intermittent failing test.

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8522894 [details] [diff] [review]
part0: fixing up inspector test

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

LGTM. As discussed on IRC, this will make the test more robust.

Comment 32

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c87c021a5092

Comment 33

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/53e97446fed9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c638dcabef9a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/044d2a98a497

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/53e97446fed9
https://hg.mozilla.org/mozilla-central/rev/c638dcabef9a
https://hg.mozilla.org/mozilla-central/rev/044d2a98a497