Creating custom element with setElementCreationCallback makes microtasks run synchronously
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
People
(Reporter: Oriol, Unassigned)
References
(Regression)
Details
(Keywords: regression)
Run this in the browser console:
queueMicrotask(() => console.log(2));
document.createXULElement("browser");
console.log(1);
Expected:
1
2
Actual:
2
1
I don't think the microtask should be affected by that??
Reporter | ||
Comment 1•3 years ago
|
||
Using Promise.resolve().then
instead of queueMicrotask
, I found this regression window:
Suspect: bug 1441935
Updated•3 years ago
|
Updated•3 years ago
|
Comment 2•3 years ago
|
||
:bgrins, since you are the author of the regressor, bug 1441935, could you take a look?
For more information, please visit auto_nag documentation.
Updated•3 years ago
|
Reporter | ||
Comment 3•3 years ago
|
||
This is actually due to setElementCreationCallback
:
customElements.setElementCreationCallback("foo-bar", () => {});
queueMicrotask(() => console.log(2));
document.createElement("foo-bar");
console.log(1);
is wrong, but removing the 1st line avoids the problem.
Reporter | ||
Comment 4•3 years ago
|
||
...so the actual regression window is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1800b8895c08bc0c60302775dc0a4b5ea4deb310&tochange=24bae072acb09114c367e6b9ffde9261b2ad8a58
That's bug 1460815, so setElementCreationCallback
has had this problem since it was added.
Comment 5•3 years ago
|
||
(In reply to Oriol Brufau [:Oriol] from comment #0)
I don't think the microtask should be affected by that??
What's the practical impact here, ie why does it matter that microtasks execute in this case?
(In reply to Oriol Brufau [:Oriol] from comment #4)
...so the actual regression window is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1800b8895c08bc0c60302775dc0a4b5ea4deb310&tochange=24bae072acb09114c367e6b9ffde9261b2ad8a58
That's bug 1460815, so
setElementCreationCallback
has had this problem since it was added.
Tim's bugzilla account has no activity in the past 18 months, so I don't know that pinging him is likely to get you an answer.
ISTM that :arai might be in the best position to answer questions about why microtasks execute here and if there are alternative possibilities for the implementation of setElementCreationCallback
that wouldn't run microtasks - and if that is even something we want.
Reporter | ||
Comment 6•3 years ago
|
||
(In reply to :Gijs (he/him) from comment #5)
What's the practical impact here, ie why does it matter that microtasks execute in this case?
I wanted to await a tick in _setPositionalAttributes()
for bug 1760461. I already used this approach for multiselections (bug 1580003).
But there is some document.createXULElement("browser")
that makes this approach useless.
I can still use requestAnimationFrame
or something, but queueMicrotask
or Promise.resolve().then
seem safer.
Comment 7•3 years ago
|
||
There's tons of APIs that can run microtasks. I think it would be better to use requestAnimationFrame
to coalesce stuff before it gets displayed, it seems closer to what you really want right?
Reporter | ||
Comment 8•3 years ago
|
||
Yeah, I don't think this is blocking bug 1760461, but my 1st idea was that a microtask would be less likely to break things, and running microtasks synchronously seems a bug anyway.
Comment 9•3 years ago
|
||
The issue is slightly different between the original one in bug 1760461 and this bug in browser console.
the microtask checkpoint is performed if CycleCollectedJSContext::mMicroTaskLevel
becomes 0 when leaving microtask:
- for
createXULElement
case in this bug, it happens inCustomElementConstructor::Construct
'sCallSetup
dtor - for
createElement
withsetElementCreationCallback
case in this bug, it happens inCustomElementCreationCallback::Call
'sCallSetup
dtor
but the reason why it's 0 should be different between browser console and bug 1760461.
For browser console, it's simply because we don't enter microtask before evaluating the console input (I'm not yet sure whether that's reasonable or not),
so, the microtask entered inside the CallSetup
there is supposed to be the outermost one.
For bug 1760461's case, there should be some different reason, because so far I've confirmed that the current this.tabContainer._setPositionalAttributes() is done within mMicroTaskLevel = 2
,
so the microtask enqueued there won't be immediately performed, at least until it leaves those 2 microtasks.
For browser console issue, I'll investigate if entering microtask before evaluating the console input is what we want.
For bug 1760461's case, can you provide a testcase patch that reproduces the issue?
Comment 10•3 years ago
|
||
<del>
Looking at the original patch and comment 3, perhaps there is an alternative implementation of CustomElementRegistry::RunCustomElementCreationCallback::Run()
that could run the callback without running the micro-task queued?
I don't know Gecko/SpiderMonkey enough to tell if that's possible or not. Also be aware changing the behavior here may break things that accidentally depends on it.
I won't be able to investigate further since I am no longer active. Sorry!
</del>
Hum it probably don't make sense to ask JS engine to run a macro-task without running micro-task enqueued. That breaks so many assumptions in the macro-task...
Comment 11•3 years ago
|
||
Or you can follow what's said in comment 7, recognizing the behavior as intended...
This should not be visible on the Web content; Web content do not have access to setElementCreationCallback
API.
Updated•3 years ago
|
Reporter | ||
Comment 12•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #9)
For bug 1760461's case, can you provide a testcase patch that reproduces the issue?
Apply this patch:
--- a/browser/base/content/tabbrowser-tabs.js
+++ b/browser/base/content/tabbrowser-tabs.js
@@ -1173,6 +1173,10 @@
}
_setPositionalAttributes() {
+ console.log("A");
+ Promise.resolve().then(() => {
+ console.log("B");
+ });
let visibleTabs = this._getVisibleTabs();
if (!visibleTabs.length) {
return;
Compile, run, open the browser console and execute
var params = { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal() };
for (let i = 0; i < 5; ++i)
gBrowser.addTab("about:blank", params);
console.log("---");
You will see
A
B
A
B
A
B
A
B
A
B
---
It should be
A
A
A
A
A
---
B
B
B
B
B
This is because gBrowser.addTab
calls gBrowser.createBrowser
, which runs document.createXULElement("browser")
, and this runs the async console.log("B")
.
Comment 13•3 years ago
|
||
Do you have testcase without using browser console?
as noted above, browser console doesn't work well with microtask checkpoint,
so it's known that the issue happens there.
I wonder if the same issue happens also if the code is integrated into the browser itself.
(and if the issue is specific to browser console, it won't block your patch I think?)
Reporter | ||
Comment 14•3 years ago
|
||
I can still trigger some strange behavior without the browser console:
- Apply this patch and compile:
diff --git a/browser/base/content/tabbrowser-tabs.js b/browser/base/content/tabbrowser-tabs.js
--- a/browser/base/content/tabbrowser-tabs.js
+++ b/browser/base/content/tabbrowser-tabs.js
@@ -1173,6 +1173,12 @@
}
_setPositionalAttributes() {
+ this._id = (this._id || 0) + 1;
+ const id = this._id;
+ console.log("A", id);
+ Promise.resolve().then(() => {
+ console.log("B", id);
+ });
let visibleTabs = this._getVisibleTabs();
if (!visibleTabs.length) {
return;
diff --git a/browser/base/content/tabbrowser.js b/browser/base/content/tabbrowser.js
--- a/browser/base/content/tabbrowser.js
+++ b/browser/base/content/tabbrowser.js
@@ -3440,7 +3440,9 @@
return;
}
+ console.log("START");
this.removeTabs(selectedTabs);
+ console.log("END");
},
/**
- Set this pref in about:config:
ui.prefersReducedMotion = 1
. It doesn't exist by default, you may have to create it as a number. - Make sure a window has a few tabs, e.g. 5 tabs
- Multiselect 4 tabs with ctrl or shift
- Right click a multiselected tab, choose "Close 4 Tabs"
Then if you open the browser console to see the results:
START
A 4
A 5
A 6
B 4
B 5
B 6
A 7
A 8
END
B 7
B 8
So, some async B were forced to be run synchronously before completing gbrowser.removeTabs()
.
That said, most A happen first, so in this case it would be good enough for bug 1760461. I will probably use requestAnimationFrame
anyways.
Comment 15•3 years ago
|
||
Thank you for the testcase.
This issue is unrelated to setElementCreationCallback
or callback or microtask level.
Here's the JS backtrace of _setPositionalAttributes
function:
0 chrome://browser/content/tabbrowser-tabs.js MozTabbrowserTabs._setPositionalAttributes
1 chrome://browser/content/tabbrowser.js window._gBrowser._endRemoveTab
2 chrome://browser/content/tabbrowser.js window._gBrowser._removeTab
3 chrome://browser/content/tabbrowser.js window._gBrowser._startRemoveTabs
4 chrome://browser/content/tabbrowser.js window._gBrowser.removeTabs
5 chrome://browser/content/tabbrowser.js window._gBrowser.removeMultiSelectedTabs
6 chrome://browser/content/tabbrowser.js window._gBrowser.closeContextTabs
7 chrome://browser/content/browser.xhtml
And the partial backtrace of console.log("B", id)
executed before "END"
,
where microtask checkpoint (10-th frame) happens inside nsThreadManager::SpinEventLoopUntilInternal
.
0 Interpret(JSContext*, js::RunState&) + 26628
1 js::RunScript(JSContext*, js::RunState&) + 348
2 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) + 1580
3 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) + 212
4 PromiseReactionJob(JSContext*, unsigned int, JS::Value*) + 1472
5 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) + 740
6 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) + 212
7 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) + 340
8 mozilla::dom::PromiseJobCallback::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::ErrorResult&) + 160
9 mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) + 248
10 mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) + 956
11 mozilla::CycleCollectedJSContext::BeforeProcessTask(bool) + 20
12 nsThread::ProcessNextEvent(bool, bool*) + 532
13 nsThreadManager::SpinEventLoopUntilInternal(nsTSubstring<char> const&, nsINestedEventLoopCondition*, mozilla::ShutdownPhase) + 460
14 _NS_InvokeByIndex + 96
15 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 3632
16 XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) + 588
17 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) + 740
That's triggered by the following Services.tm.spinEventLoopUntilOrQuit
call in window._gBrowser.removeTabs
:
window._gBrowser = {
...
removeTabs(
tabs,
{
animate = true,
suppressWarnAboutClosingWindow = false,
skipPermitUnload = false,
} = {}
) {
...
Services.tm.spinEventLoopUntilOrQuit(
"tabbrowser.js:removeTabs",
() => done || window.closed
);
in the following JS backtrace:
0 chrome://browser/content/tabbrowser.js window._gBrowser.removeTabs
1 chrome://browser/content/tabbrowser.js window._gBrowser.removeMultiSelectedTabs
2 chrome://browser/content/tabbrowser.js window._gBrowser.closeContextTabs
3 chrome://browser/content/browser.xhtml
So, removeTabs
's code explicitly spins the event loop and performs microtask checkpoint as a part of processing tasks there,
that's why the microtasks queued there is performed before "END"
.
Updated•3 years ago
|
Comment 16•3 years ago
|
||
For browser console's case, filed bug 1761590 for devtools.
Updated•3 years ago
|
Comment 17•3 years ago
|
||
So is there anything DOM specific here? Or should we just close this?
bug 1761590 is about devtools and the other testcase is about frontend internal event loop spinning.
Reporter | ||
Comment 18•3 years ago
|
||
Yes with bug 1761590 I guess this can be closed.
Updated•3 years ago
|
Updated•3 years ago
|
Description
•