Bug 147933 - Promise callbacks should be called at microtask checkpoints
Summary: Promise callbacks should be called at microtask checkpoints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
: 149003 150867 150976 (view as bug list)
Depends on:
Blocks: 140043 149032
  Show dependency treegraph
 
Reported: 2015-08-12 04:49 PDT by Jake Archibald
Modified: 2016-08-29 13:54 PDT (History)
13 users (show)

See Also:


Attachments
Patch (68.66 KB, patch)
2015-08-28 08:59 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
WIP (68.86 KB, patch)
2015-08-30 03:47 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (689.66 KB, application/zip)
2015-08-30 04:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (690.60 KB, application/zip)
2015-08-30 04:36 PDT, Build Bot
no flags Details
WIP (69.39 KB, patch)
2015-08-30 11:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (549.62 KB, application/zip)
2015-08-30 12:01 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (714.68 KB, application/zip)
2015-08-30 12:07 PDT, Build Bot
no flags Details
Patch (68.84 KB, patch)
2015-10-10 15:36 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (69.18 KB, patch)
2015-10-11 10:16 PDT, Sam Weinig
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jake Archibald 2015-08-12 04:49:47 PDT
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.
Comment 1 Yusuke Suzuki 2015-08-12 17:26:27 PDT
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.
Comment 2 Yusuke Suzuki 2015-08-12 18:07:01 PDT
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
Comment 3 Sam Weinig 2015-08-28 08:59:21 PDT
Created attachment 260155 [details]
Patch
Comment 4 Sam Weinig 2015-08-28 09:08:11 PDT
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.
Comment 5 Sam Weinig 2015-08-28 09:09:54 PDT
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.
Comment 6 Sam Weinig 2015-08-30 03:47:40 PDT
Created attachment 260239 [details]
WIP
Comment 7 Build Bot 2015-08-30 04:31:55 PDT
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.
Comment 8 Build Bot 2015-08-30 04:31:58 PDT
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
Comment 9 Build Bot 2015-08-30 04:36:49 PDT
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.
Comment 10 Build Bot 2015-08-30 04:36:53 PDT
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
Comment 11 Sam Weinig 2015-08-30 11:18:37 PDT
Created attachment 260244 [details]
WIP
Comment 12 Build Bot 2015-08-30 12:01:10 PDT
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
Comment 13 Build Bot 2015-08-30 12:01:13 PDT
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
Comment 14 Build Bot 2015-08-30 12:06:59 PDT
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
Comment 15 Build Bot 2015-08-30 12:07:02 PDT
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
Comment 16 Yoav Weiss 2015-08-31 02:50:45 PDT
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)
Comment 17 Yoav Weiss 2015-08-31 02:59:04 PDT
(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).
Comment 18 Sam Weinig 2015-08-31 11:47:53 PDT
(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?
Comment 19 Yoav Weiss 2015-08-31 12:44:54 PDT
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.
Comment 20 Sam Weinig 2015-08-31 12:54:31 PDT
(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.
Comment 21 Yoav Weiss 2015-08-31 13:57:45 PDT
(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
Comment 22 Sam Weinig 2015-09-09 15:52:41 PDT
*** Bug 149003 has been marked as a duplicate of this bug. ***
Comment 23 Sam Weinig 2015-10-10 15:36:49 PDT
Created attachment 262835 [details]
Patch
Comment 24 Sam Weinig 2015-10-11 10:16:51 PDT
Created attachment 262865 [details]
Patch
Comment 25 Chris Dumez 2015-10-15 11:17:25 PDT
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?
Comment 26 Yoav Weiss 2015-10-19 01:37:06 PDT
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?
Comment 27 Xabier Rodríguez Calvar 2015-11-04 01:36:42 PST
*** Bug 150867 has been marked as a duplicate of this bug. ***
Comment 28 Xabier Rodríguez Calvar 2015-11-09 01:58:16 PST
*** Bug 150976 has been marked as a duplicate of this bug. ***
Comment 29 Radar WebKit Bug Importer 2015-12-02 14:28:51 PST
<rdar://problem/23731553>
Comment 30 Sam Weinig 2015-12-03 11:18:41 PST
Landed in https://trac.webkit.org/r193286.
Comment 31 Xabier Rodríguez Calvar 2015-12-07 11:21:54 PST
Committed r193641: <http://trac.webkit.org/changeset/193641>
Comment 32 Alexey Proskuryakov 2016-08-29 13:54:40 PDT
Looks like this made WebKit behave differently from Firefox and Chrome in at least once case, see bug 161291.