Add more tests for Clients.claim()

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Ehsan, Assigned: HoPang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 14 obsolete attachments)

9.07 KB, patch
HoPang
: review+
Details | Diff | Splinter Review
7.27 KB, patch
HoPang
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
This is a follow-up to bug 1130684.
(Reporter)

Updated

3 years ago
Blocks: 1130684
(Assignee)

Updated

a year ago
Assignee: nobody → bhsu
(Assignee)

Comment 1

a year ago
Created attachment 8823181 [details] [diff] [review]
Part 1: Migrate claim_fetch_worker to wpt

I think claim_fetch_worker is the testcase requested for testing a fetch request should be intercepted by a service worker after its originating window is claimed by the service worker.

In addition to the migration, I slightly update the testcase to make it a little more reasonable. The test iframe is now created before registering a service worker. Otherwise, once the registration becomes ready, the iframe will be created with its controller set to the active service worker of the registration, and I don't think that's what we want.
(Assignee)

Comment 2

a year ago
Created attachment 8823185 [details] [diff] [review]
Part 1: Migrate claim_fetch_worker to wpt

re-upload with mercurial format :P

Please refer to comment 1 for more detailed information.
Attachment #8823181 - Attachment is obsolete: true
(Assignee)

Comment 3

a year ago
Created attachment 8823199 [details] [diff] [review]
Part 2: Create a testcase for claim() should affect other registrations

This is the testcase for a service worker registration should be affected if the only client of its active service worker is claimed away by another service worker. For example, its waiting service worker should become the active service worker as [0] suggests.

I'd like to implement the scenario [1] suggesting as well. However, once a registration is unregistered, we cannot find it via `navigator.serviceWorker.getRegistration(<SCOPE>)`, because its uninstalling flag is set.

[0] https://github.com/w3c/ServiceWorker/issues/682#issuecomment-95229504
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1130684#c7
(Assignee)

Comment 4

a year ago
Created attachment 8823203 [details] [diff] [review]
Part 1: Migrate claim_fetch_worker to wpt

Upload a passable version.
Attachment #8823185 - Attachment is obsolete: true
(Assignee)

Comment 5

a year ago
Created attachment 8823204 [details] [diff] [review]
Part 2: Create a testcase for claim() should affect other registrations
Attachment #8823199 - Attachment is obsolete: true
(Assignee)

Comment 6

a year ago
Hello Ben,

Sorry for keep spamming, the details of the two testcases remain the same as mentioned in comment 1 and comment 3, respectively.

The two testcases are now uploaded in a passable form, though I personally don't suggest using the two testcases directly. There is a test in both testcases rewrote in a passable form, IMHO, we should compare the service worker object instead of their scriptURL in the following code snippet, but doing so fails the two testcases.

```
assert_equals(
  frame.contentWindow.navigator.serviceWorker.controller.scriptURL,
  worker.scriptURL, 	
  'Test iframe should be claimed.');
```

I think we can file a following bug to fix this, and update the testcases correspondingly in that bug. How do you think?
Flags: needinfo?(bkelly)
So, the reason you have to compare scriptURL here is that:

 frame.contentWindow.navigator.serviceWorker.controller

Was created with a different global from `worker`.  We can't have js object equality across compartments.  So there are two different js objects representing the same worker.

So if you want to do direct object comparison you need to get the registration.installing from the frame global in order to compare later with controller.  Not sure how easy that is to do with the test, though.
Flags: needinfo?(bkelly)
(Assignee)

Comment 8

a year ago
Thanks, It's great to know that it is because of JS compartment, which I've never thought about. I believe getting the service worker within the iframe is totally doable, and I'll update the patches correspondingly since comparing the objects is much more meaningful than comparing merely the URLs.
(Assignee)

Comment 9

a year ago
Created attachment 8823549 [details] [diff] [review]
Part 1: Migrate claim_fetch_worker to wpt
Attachment #8823203 - Attachment is obsolete: true
Attachment #8823549 - Flags: review?(bkelly)
(Assignee)

Comment 10

a year ago
Created attachment 8823550 [details] [diff] [review]
Part 2: Create a testcase for claim() should affect other registrations
Attachment #8823204 - Attachment is obsolete: true
Attachment #8823550 - Flags: review?(bkelly)

Updated

a year ago
Blocks: 1328614
No longer blocks: 1173500

Updated

a year ago
Attachment #8823549 - Flags: review?(bkelly) → review+

Updated

a year ago
Attachment #8823550 - Flags: review?(bkelly) → review+
(Assignee)

Comment 11

a year ago
Created attachment 8826101 [details] [diff] [review]
(V2) Part 1: Migrate claim_fetch_worker to wpt

rebased
Attachment #8823549 - Attachment is obsolete: true
Attachment #8826101 - Flags: review+
(Assignee)

Comment 12

a year ago
Created attachment 8826102 [details] [diff] [review]
(V2) Part 2: Create a testcase for claim() should affect other registrations

rebased
Attachment #8823550 - Attachment is obsolete: true
Attachment #8826102 - Flags: review+
(Assignee)

Comment 13

a year ago
Created attachment 8829716 [details] [diff] [review]
(V3) Part 2: Create a testcase for claim() should affect other registrations

The previous version fails intermittently, since service workers move from activating to activated automatically, and thus it maybe not appropriate to check whether a service worker is of activating state.
Attachment #8826102 - Attachment is obsolete: true
Attachment #8829716 - Flags: review+
Needs rebasing.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Needs rebasing.

HoPang, it's a kind reminder in case you didn't notice comment 15.
Flags: needinfo?(bhsu)
(Assignee)

Comment 17

a year ago
Thanks, I've already rebased them and push the to try last week.
Flags: needinfo?(bhsu)
(Assignee)

Comment 18

a year ago
Created attachment 8839305 [details] [diff] [review]
(V3) Part 1: Migrate claim_fetch_worker to wpt
Attachment #8826101 - Attachment is obsolete: true
Attachment #8839305 - Flags: review+
(Assignee)

Comment 19

a year ago
Created attachment 8839306 [details] [diff] [review]
(V4) Part 2: Create a testcase for claim() should affect other registrations
Attachment #8829716 - Attachment is obsolete: true
Attachment #8839306 - Flags: review+
(Assignee)

Comment 20

a year ago
Created attachment 8841162 [details] [diff] [review]
Part 1: Migrate claim_fetch_worker to wpt
(Assignee)

Comment 21

a year ago
Created attachment 8841164 [details] [diff] [review]
Part 2: Create a testcase for claim() should affect other registrations
(Assignee)

Updated

a year ago
Attachment #8839305 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8839306 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8841162 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8841164 - Attachment is obsolete: true
(Assignee)

Comment 22

a year ago
Created attachment 8842273 [details] [diff] [review]
(V4) Part 1: Migrate claim_fetch_worker to wpt
(Assignee)

Comment 23

a year ago
Created attachment 8842275 [details] [diff] [review]
(V5) Part 2: Create a testcase for claim() should affect other registrations
(Assignee)

Comment 24

a year ago
Comment on attachment 8842273 [details] [diff] [review]
(V4) Part 1: Migrate claim_fetch_worker to wpt

Another rebased version.
Attachment #8842273 - Attachment description: Part 1: Migrate claim_fetch_worker to wpt → (V4) Part 1: Migrate claim_fetch_worker to wpt
Attachment #8842273 - Flags: review+
(Assignee)

Comment 25

a year ago
Comment on attachment 8842275 [details] [diff] [review]
(V5) Part 2: Create a testcase for claim() should affect other registrations

Another rebased version.
Attachment #8842275 - Attachment description: Part 2: Create a testcase for claim() should affect other registrations → (V5) Part 2: Create a testcase for claim() should affect other registrations
(Assignee)

Updated

a year ago
Attachment #8842275 - Flags: review+

Comment 27

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b719e03aa6f8
Part 1: Migrate claim_fetch_worker to wpt. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b9fda58dd5
Part 2: Create a testcase for claim() should affect other registrations. r=bkelly
Keywords: checkin-needed

Comment 28

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b719e03aa6f8
https://hg.mozilla.org/mozilla-central/rev/e7b9fda58dd5
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.