Bug 147933

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.

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

Created attachment 260155 [details] Patch

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.

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.

Created attachment 260239 [details] WIP

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.

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

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

Created attachment 260244 [details] WIP

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

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

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

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

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

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

(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

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

Created attachment 262835 [details] Patch

Created attachment 262865 [details] Patch

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

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

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

<rdar://problem/23731553>

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

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

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