Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update service_worker_unregister_and_register() #19613

Merged
merged 2 commits into from Oct 21, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Oct 10, 2019

Allows callsites of this helper function to pass RegistrationOptions.
It will be useful for testing module service workers.

Bug: 824647
Change-Id: Idc9a5c012144da46dfc4d692ce26df9f604a6ea1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1851624
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704963}


This PR also includes the following the Chromium CL:

Update service_worker_unregister_and_register()

Allows callsites of this helper function to pass RegistrationOptions.
It will be useful for testing module service workers.

Bug: N/A
Change-Id: Ia6fae23881704e4eb41e33ebe3e22e89872d7f87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1866093
Commit-Queue: Kenichi Ishibashi bashi@chromium.org
Reviewed-by: Hiroki Nakagawa nhiroki@chromium.org
Cr-Commit-Position: refs/heads/master@{#707713}

Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1851624 branch 2 times, most recently from 3f9505b to 8e6b06b Oct 10, 2019
Allows callsites of this helper function to pass RegistrationOptions.
It will be useful for testing module service workers.

Bug: 824647
Change-Id: Idc9a5c012144da46dfc4d692ce26df9f604a6ea1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1851624
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704963}
@KyleJu

This comment has been minimized.

Copy link

KyleJu commented Oct 11, 2019

The followings are unstable results on Chrome

Unstable results

Test Subtest Results Messages
/background-fetch/fetch-uploads.https.window.html ERROR: 1/10, TIMEOUT: 9/10
/background-fetch/fetch-uploads.https.window.html Fetch with an upload should work PASS: 6/10, TIMEOUT: 3/10, MISSING: 1/10 Test timed out
/background-fetch/fetch-uploads.https.window.html Progress event includes uploaded bytes NOTRUN: 3/10, TIMEOUT: 6/10, MISSING: 1/10 Test timed out
/background-fetch/fetch-uploads.https.window.html Duplicate upload requests work and can be distinguished. NOTRUN: 9/10, MISSING: 1/10
/fetch/range/sw.https.window.html OK: 5/10, TIMEOUT: 5/10
/fetch/range/sw.https.window.html Defer range header passthrough tests to service worker PASS: 6/10, TIMEOUT: 4/10 Test timed out
/fetch/range/sw.https.window.html Ranged response not allowed following no-cors ranged request FAIL: 5/10, NOTRUN: 4/10, TIMEOUT: 1/10 assert_unreached: Should have rejected: undefined Reached unreachable code;Test timed out
/fetch/range/sw.https.window.html Non-opaque ranged response executed FAIL: 5/10, NOTRUN: 5/10 promise_test: Unhandled rejection with value: object "Error: Script load failed"
/fetch/range/sw.https.window.html Accept-Encoding should not appear in a service worker FAIL: 5/10, NOTRUN: 5/10 assert_equals: Accept-Encoding should not be set for media expected (object) null but got (string) "identity;q=1, *;q=0"
/fetch/range/sw.https.window.html Include range header in network request FAIL: 6/10, TIMEOUT: 4/10 assert_equals: expected (string) "range-header-received" but got (object) null;Test timed out
/push-api/idlharness.https.any.sharedworker.html OK: 9/10, TIMEOUT: 1/10
/push-api/idlharness.https.any.sharedworker.html idl_test setup PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html idl_test validation PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html Partial interface ServiceWorkerRegistration: original interface defined PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html Partial interface ServiceWorkerRegistration: member names are unique PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html Partial interface ServiceWorkerGlobalScope: original interface defined PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html Partial interface ServiceWorkerGlobalScope: valid exposure set PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html Partial interface ServiceWorkerGlobalScope: member names are unique PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html WorkerGlobalScope includes WindowOrWorkerGlobalScope: member names are unique PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface object length PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface object name PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: existence and properties of interface prototype object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: existence and properties of interface prototype object's "constructor" property PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: existence and properties of interface prototype object's @@unscopables property PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: attribute supportedContentEncodings PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: operation subscribe(PushSubscriptionOptionsInit) PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: operation getSubscription() PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushManager interface: operation permissionState(PushSubscriptionOptionsInit) PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface object length PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface object name PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface: existence and properties of interface prototype object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface: existence and properties of interface prototype object's "constructor" property PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface: existence and properties of interface prototype object's @@unscopables property PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface: attribute userVisibleOnly PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionOptions interface: attribute applicationServerKey PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface object length PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface object name PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: existence and properties of interface prototype object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: existence and properties of interface prototype object's "constructor" property PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: existence and properties of interface prototype object's @@unscopables property PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: attribute endpoint PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: attribute expirationTime PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: attribute options PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: operation getKey(PushEncryptionKeyName) PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: operation unsubscribe() PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscription interface: operation toJSON() PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushMessageData interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushEvent interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html PushSubscriptionChangeEvent interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html ServiceWorkerRegistration interface: attribute pushManager PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html ServiceWorkerGlobalScope interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/push-api/idlharness.https.any.sharedworker.html ExtendableEvent interface: existence and properties of interface object PASS: 9/10, MISSING: 1/10
/service-workers/cache-storage/serviceworker/cache-add.https.html Cache.addAll should succeed when entries differ by vary header FAIL: 9/10, PASS: 1/10 promise_test: Unhandled rejection with value: object "TypeError: Request failed"
/service-workers/cache-storage/serviceworker/cache-add.https.html Cache.addAll should reject when entries are duplicate by vary header FAIL: 9/10, PASS: 1/10 assert_throws: Cache.addAll() should reject when entries are duplicate by vary header function "function() { throw e }" threw object "TypeError: Request failed" that is not a DOMException InvalidStateError: property "code" is equal to undefined, expected 11
/service-workers/service-worker/claim-not-using-registration.https.html Test claim client when there's a longer-matched registration not already used by the page FAIL: 5/10, PASS: 5/10 assert_equals: Frame should not be claimed when a longer-matched registration exists expected null but got object "[object ServiceWorker]"
/service-workers/service-worker/worker-interception.https.html Verify a cors worker script served by a service worker fails shared worker start. FAIL: 9/10, PASS: 1/10 promise_test: Unhandled rejection with value: object "Error: wait_for_state must be passed a ServiceWorker"
/service-workers/service-worker/worker-interception.https.html Verify a no-cors cross-origin worker script served by a service worker fails shared worker start. FAIL: 9/10, PASS: 1/10 promise_test: Unhandled rejection with value: object "Error: wait_for_state must be passed a ServiceWorker"
@KyleJu KyleJu closed this Oct 15, 2019
@KyleJu KyleJu reopened this Oct 15, 2019
@LukeZielinski

This comment has been minimized.

Copy link
Contributor

LukeZielinski commented Oct 16, 2019

…er()

The previous CL (crrev.com/c/1851624) seemed to make some tests flaky.
I'm not sure the exact reason. This CL attempts to fix flakiness by not
using default parameters.

Bug: N/A
Change-Id: Ia6fae23881704e4eb41e33ebe3e22e89872d7f87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1866093
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707713}
@stephenmcgruer

This comment has been minimized.

Copy link
Contributor

stephenmcgruer commented Oct 21, 2019

I'm not sure what actual flake is being introduced here. TaskCluster shows Firefox is timing out entirely (probably due to number of tests? - [taskcluster:error] Task timeout after 7200 seconds. Force killing container.), whilst Chrome lists of flakes is posted above by @KyleJu .

Running locally, using a local tip-of-tree build of Chromium, I was able to reproduce many of the flakes even without the commits in this PR. The diff is:

Without:						      |	With:

## Unstable results ##						## Unstable results ##

|                                 Test                       	|                                 Test                       
|------------------------------------------------------------	|------------------------------------------------------------
| `/background-fetch/abort.https.window.html`                	| `/background-fetch/abort.https.window.html`                
| `/background-fetch/abort.https.window.html`                	| `/background-fetch/abort.https.window.html`                
| `/background-fetch/abort.https.window.html`                	| `/background-fetch/abort.https.window.html`                
| `/background-fetch/abort.https.window.html`                	| `/background-fetch/abort.https.window.html`                
							      >	| `/background-fetch/fetch-uploads.https.window.html`        
							      >	| `/background-fetch/fetch-uploads.https.window.html`        
							      >	| `/background-fetch/fetch-uploads.https.window.html`        
							      >	| `/background-fetch/fetch-uploads.https.window.html`        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/service-workers/cache-storage/serviceworker/cache-add.htt	| `/service-workers/cache-storage/serviceworker/cache-add.htt
| `/service-workers/cache-storage/serviceworker/cache-add.htt	| `/service-workers/cache-storage/serviceworker/cache-add.htt
| `/service-workers/service-worker/worker-interception.https.	| `/service-workers/service-worker/worker-interception.https.
| `/service-workers/service-worker/worker-interception.https.  	| `/service-workers/service-worker/worker-interception.https.

So only the /background-fetch/fetch-uploads.https.window.html failures appear to be new here, at least based off my machine. When they flake, I see:

| `/background-fetch/fetch-uploads.https.window.html`                 |                                                                                                       | **ERROR: 1/10, TIMEOUT: 9/10**              |                                                                                                                                                                                                                                                                     |
| `/background-fetch/fetch-uploads.https.window.html`                 | `Fetch with an upload should work`                                                                    | **TIMEOUT: 9/10, MISSING: 1/10**            | `Test timed out`                                                                                                                                                                                                                                                    |
| `/background-fetch/fetch-uploads.https.window.html`                 | `Progress event includes uploaded bytes`                                                              | **NOTRUN: 9/10, MISSING: 1/10**             |                                                                                                                                                                                                                                                                     |
| `/background-fetch/fetch-uploads.https.window.html`                 | `Duplicate upload requests work and can be distinguished.`                                            | **NOTRUN: 9/10, MISSING: 1/10**             | 

I'm not actually convinced that even this is a flake caused by this CL; these tests just seem very flaky in general...

@Hexcles @foolip as FYI

@stephenmcgruer

This comment has been minimized.

Copy link
Contributor

stephenmcgruer commented Oct 21, 2019

A second run of without-pr and with-pr shows /background-fetch/fetch-uploads.https.window.html coming up as newly flaking for Chrome tot at least.

I also ran before/after with Firefox and saw no difference in flaking. With or without the CLs in this PR, the following tests flaked:

## Unstable results ##						## Unstable results ##

|                                 Test                       	|                                 Test                       
|------------------------------------------------------------	|------------------------------------------------------------
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/fetch/range/sw.https.window.html`                        	| `/fetch/range/sw.https.window.html`                        
| `/service-workers/cache-storage/serviceworker/cache-add.htt	| `/service-workers/cache-storage/serviceworker/cache-add.htt
| `/service-workers/cache-storage/serviceworker/cache-add.htt	| `/service-workers/cache-storage/serviceworker/cache-add.htt
@Hexcles

This comment has been minimized.

Copy link
Member

Hexcles commented Oct 21, 2019

Yeah I took a quick look at the patch itself and it should be pretty safe, admin-merging.

@Hexcles Hexcles merged commit 9c6848c into master Oct 21, 2019
9 of 10 checks passed
9 of 10 checks passed
Taskcluster (pull_request) TaskGroup: failure
Details
Azure Pipelines Build #20191021.23 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@Hexcles Hexcles deleted the chromium-export-cl-1851624 branch Oct 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.