Bug 188212 – Add self.queueMicrotask(f) on DOMWindow

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

See https://github.com/whatwg/html/issues/512

[Yusuke Suzuki]

Created attachment 346275 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 346275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346275&action=review

> Source/WebCore/workers/WorkerGlobalScope.cpp:256
> +    postTask([callback = WTFMove(callback)](ScriptExecutionContext&) {

How can this be correct? Microtasks & tasks are completely different concepts & timing.

[Domenic Denicola]

Spec: https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#microtask-queuing

Tests: https://github.com/web-platform-tests/wpt/pull/6334

[Ryosuke Niwa]

Comment on attachment 346275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346275&action=review

>> Source/WebCore/workers/WorkerGlobalScope.cpp:256
>> +    postTask([callback = WTFMove(callback)](ScriptExecutionContext&) {
> 
> How can this be correct? Microtasks & tasks are completely different concepts & timing.

I think we need to hook into JSC's microtask queue instead here.

[Yusuke Suzuki]

Comment on attachment 346275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346275&action=review

>>> Source/WebCore/workers/WorkerGlobalScope.cpp:256
>>> +    postTask([callback = WTFMove(callback)](ScriptExecutionContext&) {
>> 
>> How can this be correct? Microtasks & tasks are completely different concepts & timing.
> 
> I think we need to hook into JSC's microtask queue instead here.

"How can this be correct? Microtasks & tasks are completely different concepts & timing."

Yes, this is correct in terms of our current implementation.

Unfortunately, our Worker does not support Microtasks as our main thread does. So this code is the same to the current Worker's microtask queuing mechanism.
We can find that JSWorkerGlobalScopeBase::queueTaskToEventLoop does the same thing, while JSDOMWindowBase::queueTaskToEventLoop posts microtasks correctly.

"I think we need to hook into JSC's microtask queue instead here."

The current approach against using JSC's microtask queue has Pros and Cons.

Pros:
- It skips one level indirection (JSC -> WebCore through function pointer)
- It leverages binding VoidCallback type checking instead of manually typechecking in custom binding code
Cons:
- It does not use JSC's microtask stack, consistency

Given that this `self.queueMicrotask` is performance-sensitive (If it is not performance-sensitive, we can just use `Promise.resolve().then(callback)`), and I prefer non-custom binding code, I took the current approach in this patch.
But if the consistency is preferable, we can use JSC's stack in binding custom code. I'll upload this version.

[Ryosuke Niwa]

Here's a problem though.

If the worker ever supports a feature which schedules a task, then all microtasks are supposed to be drained before such a task is executed. The proposed patch doesn't implement this semantics.

I don't think we want to be in the business of auditing everything worker supports and making sure they never schedules a task.

[Yusuke Suzuki]

(In reply to Ryosuke Niwa from comment #6)
> Here's a problem though.
> 
> If the worker ever supports a feature which schedules a task, then all
> microtasks are supposed to be drained before such a task is executed. The
> proposed patch doesn't implement this semantics.
> 
> I don't think we want to be in the business of auditing everything worker
> supports and making sure they never schedules a task.

OK, then, I'll implement only DOM window version for now.

[Yusuke Suzuki]

Created attachment 346353 [details]
Patch

[Yusuke Suzuki]

Created attachment 346354 [details]
Patch

[Yusuke Suzuki]

Created attachment 346355 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 346355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review

> Source/JavaScriptCore/runtime/JSJob.cpp:-48
> -    virtual ~JSJobMicrotask()

Where did the destructor go?

> Source/WebCore/page/DOMWindow.idl:92
> +    [Custom] void queueMicrotask(VoidCallback callback);

So we're not gonna implement the one in worker??

[Ryosuke Niwa]

Comment on attachment 346355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review

> Source/JavaScriptCore/runtime/JSJob.cpp:60
> +Ref<Microtask> createJSJob(VM& vm, JSValue job)

I think we should call this createJSTask instead to use the consistent terminology.

[Yusuke Suzuki]

Comment on attachment 346355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review

>> Source/JavaScriptCore/runtime/JSJob.cpp:-48
>> -    virtual ~JSJobMicrotask()
> 
> Where did the destructor go?

Microtask has `virtual ~Microtask()`, so this is not necessary.

>> Source/WebCore/page/DOMWindow.idl:92
>> +    [Custom] void queueMicrotask(VoidCallback callback);
> 
> So we're not gonna implement the one in worker??

Yes, we will implement it after MicrotaskQueue is adopted in worker environments.

[Ryosuke Niwa]

Comment on attachment 346355 [details]
Patch

Sounds good to me.

[Ryosuke Niwa]

Let's file a bug for the worker one so that we don't forget though.

[Yusuke Suzuki]

Comment on attachment 346355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346355&action=review

>> Source/JavaScriptCore/runtime/JSJob.cpp:60
>> +Ref<Microtask> createJSJob(VM& vm, JSValue job)
> 
> I think we should call this createJSTask instead to use the consistent terminology.

The interesting thing is that this "Job" is a word used in ECMA262 :P https://tc39.github.io/ecma262/#sec-enqueuejob
Using a word "microtask" in WebKit tree consistently sounds fine. Fixed.

>>> Source/WebCore/page/DOMWindow.idl:92
>>> +    [Custom] void queueMicrotask(VoidCallback callback);
>> 
>> So we're not gonna implement the one in worker??
> 
> Yes, we will implement it after MicrotaskQueue is adopted in worker environments.

The interesting thing is that this "Job" is a word used in ECMA262 :P https://tc39.github.io/ecma262/#sec-enqueuejob
Using a word "microtask" in WebKit tree consistently sounds fine. Fixed.

[Yusuke Suzuki]

(In reply to Ryosuke Niwa from comment #15)
> Let's file a bug for the worker one so that we don't forget though.

OK, filed in bug 188246 and bug 188247

[Ryosuke Niwa]

Oh, you're right:
https://html.spec.whatwg.org/#integration-with-the-javascript-job-queue

But I think the definition of job there is slightly different from what we're doing here.

[Yusuke Suzuki]

Committed r234491: <https://trac.webkit.org/changeset/234491>

[Radar WebKit Bug Importer]

<rdar://problem/42841210>