Bug 188246 – Add support for microtasks in workers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Yusuke Suzuki]

Unfortunately, our worker does not support microtasks now!

[Yusuke Suzuki]

Created attachment 346370 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 346371 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 346372 [details]
Patch

WIP

[EWS Watchlist]

Comment on attachment 346372 [details]
Patch

Attachment 346372 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8736779

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 346376 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346372 [details]
Patch

Attachment 346372 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8736827

New failing tests:
http/tests/workers/service/install-fails.html
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html

[EWS Watchlist]

Created attachment 346377 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

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

[EWS Watchlist]

Comment on attachment 346372 [details]
Patch

Attachment 346372 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8738599

New failing tests:
fonts/monospace.html

[EWS Watchlist]

Created attachment 346385 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 346372 [details]
Patch

Attachment 346372 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8739358

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https.html
http/tests/workers/service/install-fails.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html

[EWS Watchlist]

Created attachment 346397 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Yusuke Suzuki]

Created attachment 346400 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 346405 [details]
Patch

WIP

[EWS Watchlist]

Comment on attachment 346405 [details]
Patch

Attachment 346405 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8741138

New failing tests:
http/tests/workers/service/install-fails.html
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/register-same-scope-different-script-url.https.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 346405 [details]
Patch

Attachment 346405 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8741052

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/oninstall-script-error.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/unregister-then-register-new-script.https.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-waituntil.https.html
http/tests/workers/service/install-fails.html
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html

[EWS Watchlist]

Created attachment 346415 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 346405 [details]
Patch

Attachment 346405 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8742530

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 346428 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Yusuke Suzuki]

Created attachment 346508 [details]
Patch

WIP

[EWS Watchlist]

Comment on attachment 346508 [details]
Patch

Attachment 346508 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8753608

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html

[EWS Watchlist]

Created attachment 346517 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

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

[Yusuke Suzuki]

(In reply to Build Bot from comment #21)
> Comment on attachment 346508 [details]
> Patch
> 
> Attachment 346508 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: https://webkit-queues.webkit.org/results/8753608
> 
> New failing tests:
> imported/w3c/web-platform-tests/service-workers/service-worker/extendable-
> event-async-waituntil.https.html

I looked the test failures carefully, and I think that this test checks unspecified behavior in the service worker spec.
The one of the failing test is 'no-current-extension-different-microtask'. The concept of this test is the following.

function eventHandlerInSW(event) {
    Promise.resolve().then(function () {
       event.waitUntil(Promise.resolve());  // The test expects this throws InvalidStateError.
    });
}

I cannot find why this occurs in the spec. When dispatching an event, event's dispatch flag is set.
This should be set while draining microtasks since this event handler is JS code, and this event handler exhausts JS stacks and microtask queue when finishing. This phase should be before than unsetting dispatch flag.
But maybe I'm wrong. Do you know what spec part this test checks, Ryosuke, Chris, or Youenn?

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #23)
> I looked the test failures carefully, and I think that this test checks
> unspecified behavior in the service worker spec.
> The one of the failing test is 'no-current-extension-different-microtask'.
> The concept of this test is the following.
> 
> function eventHandlerInSW(event) {
>     Promise.resolve().then(function () {
>        event.waitUntil(Promise.resolve());  // The test expects this throws
> InvalidStateError.
>     });
> }
> 
> I cannot find why this occurs in the spec. When dispatching an event,
> event's dispatch flag is set.
> This should be set while draining microtasks since this event handler is JS
> code, and this event handler exhausts JS stacks and microtask queue when
> finishing. This phase should be before than unsetting dispatch flag.
> But maybe I'm wrong. Do you know what spec part this test checks, Ryosuke,
> Chris, or Youenn?

OK, let them fail. These tests are wrong :)
https://github.com/web-platform-tests/wpt/pull/8936
https://github.com/w3c/ServiceWorker/issues/1213#issuecomment-342640579

[EWS Watchlist]

Comment on attachment 346508 [details]
Patch

Attachment 346508 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8753939

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/extendable-event-async-waituntil.https.html

[EWS Watchlist]

Created attachment 346543 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Yusuke Suzuki]

Created attachment 346544 [details]
Patch

Passing tests!

[Yusuke Suzuki]

Created attachment 346548 [details]
Patch

[Yusuke Suzuki]

Finally it's ready :D

[Yusuke Suzuki]

Comment on attachment 346548 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Rename JSMainThreadNullState to JSNullState.

Ah, I'll keep this name because,

1. This JSMainThreadNullState is only used from the main thread.
2. It includes CustomElementReactionStack, which is only effective in the main thread.

[Yusuke Suzuki]

Created attachment 346560 [details]
Patch

[Yusuke Suzuki]

Created attachment 346561 [details]
Patch

[Yusuke Suzuki]

OK, renaming is done, ready for review.

[Darin Adler]

Comment on attachment 346561 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add the support for micro tasks in workers

Grammar error here: the word "the" should be omitted

> Source/WebCore/ChangeLog:10
> +        This patch adds microtask mechanism to workers. To adopt the existing microtask mechanism in main thread,
> +        we extends JSMainThreadExecState for non-main-threads. We rename it to JSExecState, and store stacked
> +        ExecState* data in thread local storage in ThreadGlobalData instead of a static variable s_mainThreadState.

Some other minor grammar errors here. Should read:

        This patch adds the microtask mechanism to workers. To adopt the existing microtask mechanism from the main thread,
        we extend JSMainThreadExecState for non-main-threads. We rename it to JSExecState, and store stacked
        ExecState* data in thread local storage in ThreadGlobalData instead of a static variable s_mainThreadState.

> Source/WebCore/bindings/js/JSMicrotaskCallback.h:42
> +        Ref<JSMicrotaskCallback> protectedThis(*this);

Would it be nicer to just protect m_task, not the entire JSMicrotaskCallback?

Another way to write this in modern code is:

    auto protectedThis { makeRef(*this) };

> Source/WebCore/bindings/js/JSMicrotaskCallback.h:49
> +        ExecState* exec = m_globalObject->globalExec();
> +
> +        JSExecState::runTask(exec, m_task);

I’d be tempted to do this in a single line instead of using a local variable.

[Darin Adler]

Comment on attachment 346561 [details]
Patch

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

> Source/WebCore/platform/ThreadGlobalData.h:36
> +namespace JSC {
> +class ExecState;
> +} // namespace JSC
>  namespace WebCore {

I think we should have a space after this. Not sure we should comment the closing brace of this tiny thing either.

[Yusuke Suzuki]

Comment on attachment 346561 [details]
Patch

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

Thank you!

>> Source/WebCore/ChangeLog:3
>> +        Add the support for micro tasks in workers
> 
> Grammar error here: the word "the" should be omitted

Thanks, fixed.

>> Source/WebCore/ChangeLog:10
>> +        ExecState* data in thread local storage in ThreadGlobalData instead of a static variable s_mainThreadState.
> 
> Some other minor grammar errors here. Should read:
> 
>         This patch adds the microtask mechanism to workers. To adopt the existing microtask mechanism from the main thread,
>         we extend JSMainThreadExecState for non-main-threads. We rename it to JSExecState, and store stacked
>         ExecState* data in thread local storage in ThreadGlobalData instead of a static variable s_mainThreadState.

Nice, fixed.

>> Source/WebCore/bindings/js/JSMicrotaskCallback.h:42
>> +        Ref<JSMicrotaskCallback> protectedThis(*this);
> 
> Would it be nicer to just protect m_task, not the entire JSMicrotaskCallback?
> 
> Another way to write this in modern code is:
> 
>     auto protectedThis { makeRef(*this) };

Nice. Since I would like to keep this JSMicrotaskCallback protected in this patch (it prevents us from removing JSDOMGlobalObject), I would like to use `auto protectedThis { makeRef(*this) };`.
We should consider it in a separate patch. Opened! https://bugs.webkit.org/show_bug.cgi?id=188338

>> Source/WebCore/bindings/js/JSMicrotaskCallback.h:49
>> +        JSExecState::runTask(exec, m_task);
> 
> I’d be tempted to do this in a single line instead of using a local variable.

Looks nice. Fixed.

>> Source/WebCore/platform/ThreadGlobalData.h:36
>>  namespace WebCore {
> 
> I think we should have a space after this. Not sure we should comment the closing brace of this tiny thing either.

Sounds nice, fixed.

[Yusuke Suzuki]

Committed r234586: <https://trac.webkit.org/changeset/234586>

[Radar WebKit Bug Importer]

<rdar://problem/42957350>

[Truitt Savell]

It looks like <https://trac.webkit.org/changeset/234586> is causing a test to be flakey. 

imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting-installed.https.html 

test history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fskip-waiting-installed.https.html

I was unable to reproduce the flakiness on r234585 but was able to reproduce it on 234586. Out of 500 repetitions of the test around 10-20 will fail.

[Yusuke Suzuki]

(In reply to Truitt Savell from comment #39)
> It looks like <https://trac.webkit.org/changeset/234586> is causing a test
> to be flakey. 
> 
> imported/w3c/web-platform-tests/service-workers/service-worker/skip-waiting-
> installed.https.html 
> 
> test history:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-
> workers%2Fservice-worker%2Fskip-waiting-installed.https.html
> 
> I was unable to reproduce the flakiness on r234585 but was able to reproduce
> it on 234586. Out of 500 repetitions of the test around 10-20 will fail.

I'll look into this.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #40)
> I'll look into this.

It seems that this patch reveals the existing flakiness more explicitly.

Previously we run 2 macrotasks on waitUntil's promise fulfillment.
But this was wrong. We should use microtask here. The patch above fixes this.

As a result, the handler can be called faster than before.
And it may be resolved before skipWaiting's macrotask.

Even before this patch, the order is not deterministic. But in the old implementation, we used macrotasks and it accidentally makes this test non-flaky.
But essentially, this flakiness was here before this patch.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #41)
> (In reply to Yusuke Suzuki from comment #40)
> > I'll look into this.
> 
> It seems that this patch reveals the existing flakiness more explicitly.
> 
> Previously we run 2 macrotasks on waitUntil's promise fulfillment.
> But this was wrong. We should use microtask here. The patch above fixes this.
> 
> As a result, the handler can be called faster than before.
> And it may be resolved before skipWaiting's macrotask.
> 
> Even before this patch, the order is not deterministic. But in the old
> implementation, we used macrotasks and it accidentally makes this test
> non-flaky.
> But essentially, this flakiness was here before this patch.

I guess that the order checked in this test is not specified in the spec[1].

[1]: https://github.com/w3c/ServiceWorker/issues/1187#issuecomment-326488845

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #42)
> (In reply to Yusuke Suzuki from comment #41)
> > (In reply to Yusuke Suzuki from comment #40)
> > > I'll look into this.
> > 
> > It seems that this patch reveals the existing flakiness more explicitly.
> > 
> > Previously we run 2 macrotasks on waitUntil's promise fulfillment.
> > But this was wrong. We should use microtask here. The patch above fixes this.
> > 
> > As a result, the handler can be called faster than before.
> > And it may be resolved before skipWaiting's macrotask.
> > 
> > Even before this patch, the order is not deterministic. But in the old
> > implementation, we used macrotasks and it accidentally makes this test
> > non-flaky.
> > But essentially, this flakiness was here before this patch.
> 
> I guess that the order checked in this test is not specified in the spec[1].
> 
> [1]: https://github.com/w3c/ServiceWorker/issues/1187#issuecomment-326488845

We should mark this test flaky.
https://github.com/w3c/ServiceWorker/issues/1187#issuecomment-397940023

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #43)
> (In reply to Yusuke Suzuki from comment #42)
> > (In reply to Yusuke Suzuki from comment #41)
> > > (In reply to Yusuke Suzuki from comment #40)
> > > > I'll look into this.
> > > 
> > > It seems that this patch reveals the existing flakiness more explicitly.
> > > 
> > > Previously we run 2 macrotasks on waitUntil's promise fulfillment.
> > > But this was wrong. We should use microtask here. The patch above fixes this.
> > > 
> > > As a result, the handler can be called faster than before.
> > > And it may be resolved before skipWaiting's macrotask.
> > > 
> > > Even before this patch, the order is not deterministic. But in the old
> > > implementation, we used macrotasks and it accidentally makes this test
> > > non-flaky.
> > > But essentially, this flakiness was here before this patch.
> > 
> > I guess that the order checked in this test is not specified in the spec[1].
> > 
> > [1]: https://github.com/w3c/ServiceWorker/issues/1187#issuecomment-326488845
> 
> We should mark this test flaky.
> https://github.com/w3c/ServiceWorker/issues/1187#issuecomment-397940023

FYI, blink also marks it flaky. https://bugs.chromium.org/p/chromium/issues/detail?id=831509

[Yusuke Suzuki]

Committed r234654: <https://trac.webkit.org/changeset/234654>