Bug 181499 – [iOS WK2] Layout Test imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https.html is a flaky failure

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryan Haddad]

The following layout test is flaky on iOS Release WK2

imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https.html

Probable cause:

The first failure visible on the dashboard blames https://trac.webkit.org/changeset/226275/webkit

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fservice-worker%2Fupdate-after-navigation-fetch-event.https.html

--- /Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https-expected.txt
+++ /Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https-actual.txt
@@ -1,3 +1,6 @@
 
-PASS Update should be triggered after a navigation fetch event. 
 
+Harness Error (TIMEOUT), message = null
+
+TIMEOUT Update should be triggered after a navigation fetch event. Test timed out
+

[Radar WebKit Bug Importer]

<rdar://problem/36443428>

[Ryan Haddad]

Marked test as flaky in https://trac.webkit.org/changeset/227179/webkit.

[Chris Dumez]

Looking at the flakiness dashboard, it is almost all green. There is exactly one failure and it is a TEXT one, not a TIMEOUT:
--- /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https-expected.txt
+++ /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https-actual.txt
@@ -1,3 +1,3 @@
 
-PASS Update should be triggered after a navigation fetch event. 
+FAIL Update should be triggered after a navigation fetch event. promise_test: Unhandled rejection with value: object "Error: wait_for_state must be passed a ServiceWorker"

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Looking at the flakiness dashboard, it is almost all green. There is exactly
> one failure and it is a TEXT one, not a TIMEOUT:
> ---
> /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-
> results/imported/w3c/web-platform-tests/service-workers/service-worker/
> update-after-navigation-fetch-event.https-expected.txt
> +++
> /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-
> results/imported/w3c/web-platform-tests/service-workers/service-worker/
> update-after-navigation-fetch-event.https-actual.txt
> @@ -1,3 +1,3 @@
>  
> -PASS Update should be triggered after a navigation fetch event. 
> +FAIL Update should be triggered after a navigation fetch event.
> promise_test: Unhandled rejection with value: object "Error: wait_for_state
> must be passed a ServiceWorker"

The test looks like:
return service_worker_unregister_and_register(t, parsed_url, scope)
      .then(function(r) {
          registration = r;
          return wait_for_state(t, registration.installing, 'activated');
        })

wait_for_state() is given null so registration.installing is null. I believe this means that the worker is already active. This can happen when registering a service worker that is already registered. The test attempts to unregister *then* register. However, because of the "Try Clear Registration" algorithm, the registration might be kept, simply marked as uninstalling.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Looking at the flakiness dashboard, it is almost all green. There is exactly
> > one failure and it is a TEXT one, not a TIMEOUT:
> > ---
> > /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-
> > results/imported/w3c/web-platform-tests/service-workers/service-worker/
> > update-after-navigation-fetch-event.https-expected.txt
> > +++
> > /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-
> > results/imported/w3c/web-platform-tests/service-workers/service-worker/
> > update-after-navigation-fetch-event.https-actual.txt
> > @@ -1,3 +1,3 @@
> >  
> > -PASS Update should be triggered after a navigation fetch event. 
> > +FAIL Update should be triggered after a navigation fetch event.
> > promise_test: Unhandled rejection with value: object "Error: wait_for_state
> > must be passed a ServiceWorker"
> 
> The test looks like:
> return service_worker_unregister_and_register(t, parsed_url, scope)
>       .then(function(r) {
>           registration = r;
>           return wait_for_state(t, registration.installing, 'activated');
>         })
> 
> wait_for_state() is given null so registration.installing is null. I believe
> this means that the worker is already active. This can happen when
> registering a service worker that is already registered. The test attempts
> to unregister *then* register. However, because of the "Try Clear
> Registration" algorithm, the registration might be kept, simply marked as
> uninstalling.

Interestingly, this service worker is indeed used by several tests:
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/multiple-update.https.html:    var script = 'resources/update-nocookie-worker.py';
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/update-after-navigation-fetch-event.https.html:    var script = 'resources/update-nocookie-worker.py';
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/update-after-oneday.https.html:    var script = 'resources/update-nocookie-worker.py';

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > Looking at the flakiness dashboard, it is almost all green. There is exactly
> > > one failure and it is a TEXT one, not a TIMEOUT:
> > > ---
> > > /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-
> > > results/imported/w3c/web-platform-tests/service-workers/service-worker/
> > > update-after-navigation-fetch-event.https-expected.txt
> > > +++
> > > /Volumes/Data/slave/ios-simulator-11-debug-tests-wk2/build/layout-test-
> > > results/imported/w3c/web-platform-tests/service-workers/service-worker/
> > > update-after-navigation-fetch-event.https-actual.txt
> > > @@ -1,3 +1,3 @@
> > >  
> > > -PASS Update should be triggered after a navigation fetch event. 
> > > +FAIL Update should be triggered after a navigation fetch event.
> > > promise_test: Unhandled rejection with value: object "Error: wait_for_state
> > > must be passed a ServiceWorker"
> > 
> > The test looks like:
> > return service_worker_unregister_and_register(t, parsed_url, scope)
> >       .then(function(r) {
> >           registration = r;
> >           return wait_for_state(t, registration.installing, 'activated');
> >         })
> > 
> > wait_for_state() is given null so registration.installing is null. I believe
> > this means that the worker is already active. This can happen when
> > registering a service worker that is already registered. The test attempts
> > to unregister *then* register. However, because of the "Try Clear
> > Registration" algorithm, the registration might be kept, simply marked as
> > uninstalling.
> 
> Interestingly, this service worker is indeed used by several tests:
> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/
> multiple-update.https.html:    var script =
> 'resources/update-nocookie-worker.py';
> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/
> update-after-navigation-fetch-event.https.html:    var script =
> 'resources/update-nocookie-worker.py';
> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/
> update-after-oneday.https.html:    var script =
> 'resources/update-nocookie-worker.py';

Looks like I may have been wrong that the issue is with tryClear not calling clear in some cases. I was able to reproduce the test failure without having a register job reusing an existing registration.

[Chris Dumez]

Created attachment 341134 [details]
Patch

[Chris Dumez]

Created attachment 341145 [details]
Patch

[EWS Watchlist]

Comment on attachment 341145 [details]
Patch

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

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 341153 [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

[Chris Dumez]

Comment on attachment 341153 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

failure is unrelated.

[youenn fablet]

Comment on attachment 341145 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp:73
> +void DeferredPromise::whenSettled(std::function<void()>&& callback)

This is more or less the same code as DOMPromise::whenSettled.
Could we try to share it?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:453
> +        });

whenSettled call is not needed if shouldNotifyWhenResolved == ShouldNotifyWhenResolved::No.
Maybe notifyWhenResolvedIfNeeded should be an optional<std::function>

[Chris Dumez]

Created attachment 341202 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 341202 [details]
Patch

Clearing flags on attachment: 341202

Committed r232156: <https://trac.webkit.org/changeset/232156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.