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

RESOLVED FIXED in mozilla36

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: krizsa, Assigned: krizsa)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 811542
Worksforme in a recentish m-c build (rev 9ee9e193fc48) with webcomponents enabled...
(Assignee)

Comment 2

3 years ago
(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)
Worksforme in a current trunk nightly too.
Created attachment 8503145 [details]
Simple testcase
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.
(Assignee)

Comment 6

3 years ago
(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.
Summary: .style attribute for customElements does not work → createdCallback is not running during the constructor when run from the scratchpad
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.
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.
(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.
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....
(Assignee)

Comment 11

3 years ago
(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?
Flags: needinfo?(bobbyholley)
It seems like AutoEntryScript should probably do a microtask, right?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Comment 14

3 years ago
Created attachment 8516949 [details] [diff] [review]
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?)
Attachment #8516949 - Flags: feedback?(bobbyholley)
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.
Attachment #8516949 - Flags: feedback?(bobbyholley) → feedback+
(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.
Promises don't use microtasks.
(Assignee)

Comment 18

3 years ago
Sigh. Still, cannot find a way to reproduce this issue in our automated testing framework...
(Assignee)

Comment 19

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=af68586aef1a
(Assignee)

Comment 20

3 years ago
Created attachment 8520774 [details] [diff] [review]
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.
Attachment #8516949 - Attachment is obsolete: true
Attachment #8520774 - Flags: review?(bobbyholley)
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?
Attachment #8520774 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 22

3 years ago
(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;
(Assignee)

Comment 23

3 years ago
Created attachment 8521202 [details] [diff] [review]
part2: Removing nsAutoMicroTask where we have AutoEntryScript. v1
Assignee: nobody → gkrizsanits
Attachment #8521202 - Flags: review?(bobbyholley)
(Assignee)

Comment 24

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d3ab34903a79
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?
(Assignee)

Comment 26

3 years ago
Created attachment 8521304 [details] [diff] [review]
part2: Removing nsAutoMicroTask where we have AutoEntryScript. v2

Removed from CallbackObject::CallSetup too.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=79da093bd7f8
Attachment #8521202 - Attachment is obsolete: true
Attachment #8521202 - Flags: review?(bobbyholley)
Attachment #8521304 - Flags: review?(bobbyholley)
Attachment #8521304 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 27

3 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f8365b8a92
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9243c59c3e56
sorry had to back out this changes for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3848635&repo=mozilla-inbound
(Assignee)

Comment 29

3 years ago
(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.
(Assignee)

Comment 30

3 years ago
Created attachment 8522894 [details] [diff] [review]
part0: fixing up inspector test

It was just a lucky coincident it seems that this test was not an intermittent failing test.
Attachment #8522894 - Flags: review?(pbrosset)
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.
Attachment #8522894 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 32

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c87c021a5092
(Assignee)

Comment 33

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/53e97446fed9
https://hg.mozilla.org/mozilla-central/rev/c638dcabef9a
https://hg.mozilla.org/mozilla-central/rev/044d2a98a497
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1100534

Updated

2 years ago
Depends on: 1149891

Updated

2 years ago
Depends on: 1169307
See Also: → bug 1222558
You need to log in before you can comment on or make changes to this bug.