Bug 147933 – Promise callbacks should be called at microtask checkpoints

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jake Archibald]

Here is a demo of the issue: http://jsbin.com/kaqume/1/edit?js,console,output

Click the blue square. The `promise` logs should appear before the `mutate` logs. The correct ordering is:

"click"
"promise"
"mutate"
"click"
"promise"
"mutate"
"timeout"
"timeout"

The microtask queue should be emptied once a listener callback has finished, before returning to spec prose. Promise callbacks seem to run on a different queue in WebKit.

[Yusuke Suzuki]

JSC Promises now just uses postTask under the WebCore environment.
It should use micro task.

Now quickly see the code base, MicroTask code in WebCore is found, but it seems that nobody except for the test uses it yet. Hm.

[Yusuke Suzuki]

Currently, MutationObserver does not use MicroTask. So to fix this,

1. When enqueuing MutationRecord at first (in this loop), we need to enqueue the micro task that later drains the mutation records[1].
2. Promises executes the asynchronous job by enqueueing the micro task.

[1]: https://dom.spec.whatwg.org/#queue-a-mutation-observer-compound-microtask

[Sam Weinig]

Created attachment 260155 [details]
Patch

[Sam Weinig]

The attached patch fixes the issue and lays some infrastructure for future improvements in our event loop model. There is one thing I am not clear on that I need to investigate further, which is are we performing the Microtask checkpoints at the right times. Previous to this patch, we had two sets of callers who seemed to be trying to implement doing work on microtask checkpoints: MutationObservers and MicroTaskQueue. They had some overlap, and some disagreement.  I decided that since our MutationObserver code has been around longer is actually used (the old microtask code is seemingly only used for testing so far), I would stick with it, but I think a more careful audit of the HTML5 spec is needed to fully understand when those checkpoints are.

[Sam Weinig]

Oh, also, I changed the name of the files to Microtasks.h/cpp so that I could fix the capitalization issue in the file name without ruining case-insensitive filesystem users day. I will change it to Microtask.h/cpp in a later patch.

[Sam Weinig]

Created attachment 260239 [details]
WIP

[Build Bot]

Comment on attachment 260239 [details]
WIP

Attachment 260239 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/122669

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 260240 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 260239 [details]
WIP

Attachment 260239 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/122673

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 260241 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Sam Weinig]

Created attachment 260244 [details]
WIP

[Build Bot]

Comment on attachment 260244 [details]
WIP

Attachment 260244 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/123573

New failing tests:
streams/reference-implementation/readable-stream.html

[Build Bot]

Created attachment 260247 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 260244 [details]
WIP

Attachment 260244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/123576

New failing tests:
streams/reference-implementation/readable-stream.html

[Build Bot]

Created attachment 260249 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yoav Weiss]

Comment on attachment 260244 [details]
WIP

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

> Source/WebCore/dom/Microtasks.cpp:101
> +    for (auto& task : m_tasksAppendedDuringMicrotaskCheckpoint)

I believe there can be a race here where microtasks are dropped to the floor, if added after all previous ones were appended. (even if Vector is thread-safe, which I'm not sure it is)

[Yoav Weiss]

(In reply to comment #4)
> The attached patch fixes the issue and lays some infrastructure for future
> improvements in our event loop model. 
> There is one thing I am not clear on
> that I need to investigate further, which is are we performing the Microtask
> checkpoints at the right times. Previous to this patch, we had two sets of
> callers who seemed to be trying to implement doing work on microtask
> checkpoints: MutationObservers and MicroTaskQueue. They had some overlap,
> and some disagreement.  I decided that since our MutationObserver code has
> been around longer is actually used (the old microtask code is seemingly
> only used for testing so far), I would stick with it, but I think a more
> careful audit of the HTML5 spec is needed to fully understand when those
> checkpoints are.

The question of where to run microtask checkpoints in indeed a question needs answering. While working on https://bugs.webkit.org/show_bug.cgi?id=134488 (which relied on the Microtasks abstraction), there were multiple cases where microtasks ended up never running (e.g. when queued from a script's timer after onload, when queued from TestRunner).

What I wanted to try next (and never got to) was to perform a microtask checkpoint from the runloop thread itself (via CFRunLoopObserver/WebCore:RunLoopObserver or otherwise).

[Sam Weinig]

(In reply to comment #16)
> Comment on attachment 260244 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260244&action=review
> 
> > Source/WebCore/dom/Microtasks.cpp:101
> > +    for (auto& task : m_tasksAppendedDuringMicrotaskCheckpoint)
> 
> I believe there can be a race here where microtasks are dropped to the
> floor, if added after all previous ones were appended. (even if Vector is
> thread-safe, which I'm not sure it is)

The entire MicrotaskQueue class is not thread-safe (in the sense that each queue can only be used from a single thread at a time). Given that, do you still think there is a race?

[Yoav Weiss]

Comment on attachment 260244 [details]
WIP

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

> Source/WebCore/dom/Microtasks.cpp:-39
> -    m_microTaskQueue.append(WTF::move(task));

Why did you remove  the `ASSERT(isMainThread());`?

>>> Source/WebCore/dom/Microtasks.cpp:101
>>> +    for (auto& task : m_tasksAppendedDuringMicrotaskCheckpoint)
>> 
>> I believe there can be a race here where microtasks are dropped to the floor, if added after all previous ones were appended. (even if Vector is thread-safe, which I'm not sure it is)
> 
> The entire MicrotaskQueue class is not thread-safe (in the sense that each queue can only be used from a single thread at a time). Given that, do you still think there is a race?

You're right. I got myself confused.
In that light, no race here AFAICT.

[Sam Weinig]

(In reply to comment #19)
> Comment on attachment 260244 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260244&action=review
> 
> > Source/WebCore/dom/Microtasks.cpp:-39
> > -    m_microTaskQueue.append(WTF::move(task));
> 
> Why did you remove  the `ASSERT(isMainThread());`?

In a future patch, MicrotaskQueue will also be used on Worker threads (though not using  the shared main thread queue), so this does not need to be only called on the main thread.

[Yoav Weiss]

(In reply to comment #20)
> (In reply to comment #19)
> > Comment on attachment 260244 [details]
> > WIP
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=260244&action=review
> > 
> > > Source/WebCore/dom/Microtasks.cpp:-39
> > > -    m_microTaskQueue.append(WTF::move(task));
> > 
> > Why did you remove  the `ASSERT(isMainThread());`?
> 
> In a future patch, MicrotaskQueue will also be used on Worker threads
> (though not using  the shared main thread queue), so this does not need to
> be only called on the main thread.

OK, makes sense

[Sam Weinig]

*** Bug 149003 has been marked as a duplicate of this bug. ***

[Sam Weinig]

Created attachment 262835 [details]
Patch

[Sam Weinig]

Created attachment 262865 [details]
Patch

[Chris Dumez]

Comment on attachment 262865 [details]
Patch

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

r=me with comments.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:217
> +    static Ref<JSDOMWindowMicrotaskCallback> create(JSDOMWindowBase* globalObject, PassRefPtr<JSC::Microtask> task)

PassRefPtr? :) We use Ref<>&&  or RefPtr<>&& nowadays.
Also, I am suspecting you don't need the JSC:: given that you already don't use it for JSC::Strong.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:229
> +        JSMainThreadExecState::runTask(exec, *m_task.get());

*m_task

.get() is redundant. Also, if m_task cannot be null, we could use a Ref<JSC::Microtask> for m_task data member and merely pass m_task here.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:235
> +    JSDOMWindowMicrotaskCallback(JSDOMWindowBase* globalObject, PassRefPtr<JSC::Microtask> task)

Ditto about PassRefPtr.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:245
> +void JSDOMWindowBase::queueTaskToEventLoop(const JSGlobalObject* object, PassRefPtr<JSC::Microtask> task)

Let's not use PassRefPtr.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:249
> +    RefPtr<JSDOMWindowMicrotaskCallback> callback = JSDOMWindowMicrotaskCallback::create((JSDOMWindowBase*)thisObject, task);

Looks like this should be a Ref<>. Also, it would be nicer to use a static_cast<>().

> Source/WebCore/dom/ActiveDOMCallbackMicrotask.h:35
> +class ActiveDOMCallbackMicrotask : public Microtask, public ActiveDOMCallback {

Could be final?

> Source/WebCore/dom/ActiveDOMCallbackMicrotask.h:46
> +    // be accessed via the ScriptExecutionContext, which should hold a reference to the relevent

typo in relevant?

> Source/WebCore/dom/Microtasks.cpp:46
> +        task->run();

Please make sure m_microTaskQueue cannot be modified by the calls to task->run() (I haven't checked).

> Source/WebCore/dom/Microtasks.cpp:63
> +    for (size_t i = 0; i < m_microtaskQueue.size(); ++i) {

m_microtaskQueue.removeFirstMatching([&task](const std::unique_ptr<MicroTask>& item) {
    return item.get() == &task;
});

> Source/WebCore/dom/Microtasks.cpp:69
> +    for (size_t i = 0; i < m_tasksAppendedDuringMicrotaskCheckpoint.size(); ++i) {

Could use Vector::removeFirstMatching().

> Source/WebCore/dom/Microtasks.h:3
> + * Copyright (C) 2015 Akamai Technologies Inc. All rights reserved.

Diff problem? This looks like an old file (not a new one). Still using the odd camel-case naming.

> Source/WebCore/dom/Microtasks.h:63
> +    WEBCORE_EXPORT void remove(const Microtask&);

Why does it need to be exported if it is private?

> Source/WebCore/dom/Microtasks.h:67
> +    bool m_performingMicrotaskCheckpoint = false;

Don't we usually use the { false } variant?

> Source/WebCore/dom/MutationObserver.cpp:166
> +    virtual ~MutationObserverMicrotask()

unnecessary I believe.

> Source/WebCore/dom/MutationObserver.cpp:171
> +    virtual Result run()

override?

> Tools/TestWebKitAPI/Tests/WebCore/Microtask.cpp:148
> +        auto* keepableRef = keepable.get();

Ref -> Ptr ?

> Tools/TestWebKitAPI/Tests/WebCore/Microtask.cpp:163
> +        auto* keepableRef = keepable.get();

Ref -> Ptr?

> LayoutTests/fast/dom/microtask-promise-mutation-observer-order.html:70
> +    for (task of tasks) {

extra curly brackets?

[Yoav Weiss]

Comment on attachment 262865 [details]
Patch

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

> Source/WebCore/bindings/js/JSMainThreadExecState.cpp:48
> +    MicrotaskQueue::mainThreadQueue().performMicrotaskCheckpoint();

Will this trigger microtask checkpoints also after running timer based functions?

[Xabier Rodríguez Calvar]

*** Bug 150867 has been marked as a duplicate of this bug. ***

[Xabier Rodríguez Calvar]

*** Bug 150976 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/23731553>

[Sam Weinig]

Landed in https://trac.webkit.org/r193286.

[Xabier Rodríguez Calvar]

Committed r193641: <http://trac.webkit.org/changeset/193641>

[Alexey Proskuryakov]

Looks like this made WebKit behave differently from Firefox and Chrome in at least once case, see bug 161291.