Description
Yusuke Suzuki
2018-08-01 20:34:37 PDT
Created attachment 346370 [details]
Patch
WIP
Created attachment 346371 [details]
Patch
WIP
Created attachment 346372 [details]
Patch
WIP
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. 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
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 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
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 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
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 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
Created attachment 346400 [details]
Patch
WIP
Created attachment 346405 [details]
Patch
WIP
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 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
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 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
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 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
Created attachment 346508 [details]
Patch
WIP
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 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
(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? (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 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 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
Created attachment 346544 [details]
Patch
Passing tests!
Created attachment 346548 [details]
Patch
Finally it's ready :D 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. Created attachment 346560 [details]
Patch
Created attachment 346561 [details]
Patch
OK, renaming is done, ready for review. 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. 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. 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. Committed r234586: <https://trac.webkit.org/changeset/234586> 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. (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. (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. (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 (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 (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 Committed r234654: <https://trac.webkit.org/changeset/234654> |