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

Jump to bottom

Update service_worker_unregister_and_register() #19613

Merged
merged 2 commits into from Oct 21, 2019
Merged

Update service_worker_unregister_and_register() #19613

merged 2 commits into from Oct 21, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Oct 10, 2019 •
edited by Hexcles

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}

wpt-pr-bot
wpt-pr-bot approved these changes Oct 10, 2019
View reviewed changes
Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Compare October 11, 2019 02:24
@bashi @chromium-wpt-export-bot
Update service_worker_unregister_and_register()
28fe38e 
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
Copy link
Contributor

KyleJu commented Oct 11, 2019 •
edited

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
Copy link
Contributor

Pinged author via https://chromium-review.googlesource.com/c/chromium/src/+/1851624

@bashi @stephenmcgruer
Stop using default parameters in service_worker_unregister_and_regist…
3b25705 
…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
Copy link
Contributor

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
Copy link
Contributor

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
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
@Hexcles Hexcles deleted the chromium-export-cl-1851624 branch October 21, 2019 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wpt-pr-bot wpt-pr-bot wpt-pr-bot approved these changes

Assignees
No one assigned
Labels
chromium-export service-workers wg-sw
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@chromium-wpt-export-bot @KyleJu @LukeZielinski @stephenmcgruer @Hexcles @wpt-pr-bot @bashi