1760791 - Creating custom element with setElementCreationCallback makes microtasks run synchronously

Status: RESOLVED INVALID

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

Description

[Oriol Brufau [:Oriol]]

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??

Comment 1

[Oriol Brufau [:Oriol]]

Using Promise.resolve().then instead of queueMicrotask, I found this regression window:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e746f670f430ceb0cb85fa8eebfe97cbe42ff01&tochange=ed68c008e5d8db0940a1fbc31bfc24efccba6c26

Suspect: bug 1441935

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:bgrins, since you are the author of the regressor, bug 1441935, could you take a look?
For more information, please visit auto_nag documentation.

Comment 3

[Oriol Brufau [:Oriol]]

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.

Comment 4

[Oriol Brufau [:Oriol]]

...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

[:Gijs (he/him)]

(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.

Comment 6

[Oriol Brufau [:Oriol]]

(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

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 8

[Oriol Brufau [:Oriol]]

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

[Tooru Fujisawa [:arai]]

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 in CustomElementConstructor::Construct's CallSetup dtor
• for createElement with setElementCreationCallback case in this bug, it happens in CustomElementCreationCallback::Call's CallSetup 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

[Tim Guan-tin Chien [:timdream] (please needinfo)]

<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

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 12

[Oriol Brufau [:Oriol]]

(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

[Tooru Fujisawa [:arai]]

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?)

Comment 14

[Oriol Brufau [:Oriol]]

I can still trigger some strange behavior without the browser console:

1. 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");
     },
 
     /**

2. Set this pref in about:config: ui.prefersReducedMotion = 1. It doesn't exist by default, you may have to create it as a number.
3. Make sure a window has a few tabs, e.g. 5 tabs
4. Multiselect 4 tabs with ctrl or shift
5. 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

[Tooru Fujisawa [:arai]]

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:

https://searchfox.org/mozilla-central/rev/3f782c2587124923a37c750b88c5a40108077057/browser/base/content/tabbrowser.js#3673-3676

  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".

Comment 16

[Tooru Fujisawa [:arai]]

For browser console's case, filed bug 1761590 for devtools.

Comment 17

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

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.

Comment 18

[Oriol Brufau [:Oriol]]

Yes with bug 1761590 I guess this can be closed.