Closed Bug 1130684 Opened 5 years ago Closed 4 years ago

implement Service Workers Clients.claim()

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bkelly, Assigned: catalinb)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 5 obsolete files)

Bug to implement a new spec feature.  See:

  https://github.com/slightlyoff/ServiceWorker/issues/586
  https://github.com/slightlyoff/ServiceWorker/commit/aeee6b9d5fa3eb071cb9667f1d599ecf90a78410

This allows a service worker to control the document on the first load of the page.  I believe this is a highly desired feature from devs.

Also, Blink has issued an intent to ship already:

  https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/bvi8fXqvNhs/uXsQHeon0IwJ
Assignee: nobody → catalin.badea392
Thanks!
Regarding the synchronization issue..
The patch I uploaded currently checks the service worker state on the main thread as follows:
1. dispatch a runnable to the main thread
2. identify source service worker
3. check if it is in an active state.
4. Continue Or Throw

On the basis that undefined behaviour gives Firefox the right to do your laundry, this is a correct implementation at the moment.
Attachment #8585689 - Attachment is obsolete: true
Attachment #8588805 - Attachment is obsolete: true
Attachment #8592500 - Flags: review?(nsm.nikhil)
Fixed a small issue.
Attachment #8592500 - Attachment is obsolete: true
Attachment #8592500 - Flags: review?(nsm.nikhil)
Attachment #8592858 - Flags: review?(nsm.nikhil)
Blocks: 1154754
Comment on attachment 8592858 [details] [diff] [review]
Implement Service Worker clients.claim.

Review of attachment 8592858 [details] [diff] [review]:
-----------------------------------------------------------------

This is on the right track.

In RuntimeService::CreateSharedWorkerFromLoadInfo, MOZ_ASSERT_IF(aType == WorkerTypeService, aLoadInfo.mServiceWorkerID > 0);

In a separate patch, add another test that claims an already open document and ensures that fetch requests before claim are not intercepted, and after claim are intercepted.

I'd like to take a look at this again after changes.

::: dom/workers/ServiceWorkerClients.cpp
@@ +159,5 @@
> +
> +    Promise* promise = mPromiseProxy->GetWorkerPromise();
> +    MOZ_ASSERT(promise);
> +
> +    if (!NS_FAILED(mResult)) {

Nit: NS_SUCCEEDED

@@ +162,5 @@
> +
> +    if (!NS_FAILED(mResult)) {
> +      promise->MaybeResolve(JS::UndefinedHandleValue);
> +    } else {
> +      promise->MaybeReject(mResult);

If result is InvalidStateError, reject with that, otherwise reject with AbortError, not what SWM returned. i.e. it would be nicer to keep the dom exposed error codes close to here rather than requiring SWM to know about them.

@@ +193,5 @@
> +    WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
> +    MOZ_ASSERT(swm);
> +    MOZ_ASSERT(workerPrivate);
> +
> +    nsresult rv = swm->ClaimClients(mScope, workerPrivate);

I'd prefer we only pass the ID to SWM rather than a ref to the worker.

@@ +298,5 @@
> +    new ClaimRunnable(promiseProxy, NS_ConvertUTF16toUTF8(scope));
> +
> +  aRv = NS_DispatchToMainThread(runnable);
> +  if (NS_WARN_IF(aRv.Failed())) {
> +    promise->MaybeReject(NS_ERROR_NOT_AVAILABLE);

Content exposed promises should be rejected only with valid DOMErrors. AbortError seems appropriate here.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2155,5 @@
> +
> +  // We keep a set of documents that service workers may choose to start
> +  // controlling using claim().
> +  MOZ_ASSERT(!mAllDocuments.Contains(aDoc));
> +  mAllDocuments.PutEntry(aDoc);

Could you move this near the top of the function since we always do this.

@@ +2171,5 @@
>    if (registration) {
> +    StopControllingADocument(registration);
> +  }
> +
> +  if (mAllDocuments.Contains(aDoc)) {

If all documents call MaybeStartControlling(), which always leads to them being added, could you assert this?

@@ +2881,1 @@
>  {

assert main thread and assert non-null aDocument.

@@ +2958,5 @@
> +{
> +  MOZ_ASSERT(aWorkerRegistration);
> +  MOZ_ASSERT(aWorkerRegistration->mActiveWorker);
> +
> +  // Same origin check

Use aWorkerRegistration->mPrincipal->Equals(aDocument->NodePrincipal()) instead for the origin check.

@@ +2990,5 @@
> +    return;
> +  }
> +
> +  if (controllingRegistration) {
> +    StopControllingADocument(controllingRegistration);

Good one! Filed https://github.com/slightlyoff/ServiceWorker/issues/682 since the spec does not actually require this yet. Could you add a test case for the situation i've outlined there. the test will go something like this:
1) just before registering '/a/b/c', call unregister on '/a'
2) after the claim call, calling getRegistration('/a/x/y') should return no registration, since /a will now go away

@@ +2994,5 @@
> +    StopControllingADocument(controllingRegistration);
> +  }
> +
> +  mControlledDocuments.Put(aDocument, aWorkerRegistration);
> +  aWorkerRegistration->StartControllingADocument();

Refactor this and the block inside MaybeStartControlling into SWM::StartControllingADocument(RegistrationInfo*)

@@ +3011,5 @@
> +    // XXXcatalinb: This can happen if the registration is unregistered while
> +    // the claim runnable is waiting in queue. We reject the claim promise
> +    // with NS_ERROR_FAILURE.
> +    NS_WARNING("Claim request doesn't have a valid registration.");
> +    return NS_ERROR_FAILURE;

Seems to me this reads exactly like "service worker is not an active worker" since the registration is gone. So you could just fail with InvalidStateError and collapse this block with the one below.

::: dom/workers/test/serviceworkers/claim_oninstall_worker.js
@@ +4,5 @@
> +  var lockState = new Promise(function(res, rej) {
> +    resolvePromise = res;
> +  });
> +
> +  e.waitUntil(lockState);

Rather than complicating things with waiting for the message, can this work?

oninstall = function(e) {
  var claimFailedPromise = new Promise(function(resolve, reject) {
    clients.claim().then(reject, resolve);
  }))
  e.waitUntil(claimFailedPromise);
}

And then, in test_claim_oninstall, simply wait until registration.installing.onstatechange occurs and make sure that one of registration.waiting and registration.active is non-null, implying that the installation succeeded, and so the call to claim failed.

::: dom/workers/test/serviceworkers/claim_worker_1.js
@@ +1,5 @@
> +onactivate = function(e) {
> +  var result = {
> +    resolve_value: false,
> +    match_count_before: -1,
> +    match_count_after: -1,

Everything except message is unused.

::: dom/workers/test/serviceworkers/claim_worker_2.js
@@ +1,5 @@
> +onactivate = function(e) {
> +  var result = {
> +    resolve_value: false,
> +    match_count_before: -1,
> +    match_count_after: -1,

ditto

::: dom/workers/test/serviceworkers/test_claim.html
@@ +106,5 @@
> +    return Promise.all([controllerChange, messageFromWorker, messageFromClient,
> +                        controllerChangeFromClient]).then(testController);
> +  }
> +
> +  function testClaimSecondWorker() {

Seems like a lot of shared code between testClaim{First,Second}Worker. It would be nice if you could refactor, but no compulsion.

@@ +135,5 @@
> +        resolveClientMessage();
> +      }
> +    }
> +
> +    return Promise.all([messageFromClient, controllerChangeFromClient])

I don't understand why controllerChange and messageFromWorker are not tested here.
Attachment #8592858 - Flags: review?(nsm.nikhil)
See Also: → 1130685
Attached file MozReview Request: bz://1130684/jaoo (obsolete) —
/r/8087 - Bug XXXXXXX: Enable some prefs for testing. r=me
/r/8089 - Bug 1130684 - Implement Service Worker clients.claim.
/r/8091 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm

Pull down these commits:

hg pull -r 8fd5e00451f5ed4e54aa54624c8c29d942796962 https://reviewboard-hg.mozilla.org/gecko/
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #8)
> Created attachment 8600982 [details]
> MozReview Request: bz://1130684/jaoo
> 
> /r/8087 - Bug XXXXXXX: Enable some prefs for testing. r=me
> /r/8089 - Bug 1130684 - Implement Service Worker clients.claim.
> /r/8091 - Bug 1131352 - Add ServiceWorkerGlobalScope skipWaiting(). r=nsm
> 
> Pull down these commits:
> 
> hg pull -r 8fd5e00451f5ed4e54aa54624c8c29d942796962
> https://reviewboard-hg.mozilla.org/gecko/

I believe this was meant for bug 1131352?
Attachment #8600982 - Attachment is obsolete: true
(In reply to Cătălin Badea (:catalinb) from comment #9)

> I believe this was meant for bug 1131352?

Yes, it does. Sorry for the noise but I'm new to this mozreview thing.
Attachment #8601765 - Flags: review?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #7)
> Comment on attachment 8592858 [details] [diff] [review]
> Implement Service Worker clients.claim.
> 
> Review of attachment 8592858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is on the right track.
> 
> In RuntimeService::CreateSharedWorkerFromLoadInfo, MOZ_ASSERT_IF(aType ==
> WorkerTypeService, aLoadInfo.mServiceWorkerID > 0);
> 
> In a separate patch, add another test that claims an already open document
> and ensures that fetch requests before claim are not intercepted, and after
> claim are intercepted.
I'll follow up with a patch.

> @@ +2171,5 @@
> >    if (registration) {
> > +    StopControllingADocument(registration);
> > +  }
> > +
> > +  if (mAllDocuments.Contains(aDoc)) {
> 
> If all documents call MaybeStartControlling(), which always leads to them
> being added, could you assert this?
That was my initial thought, too. However, the assert is hit in some chrome document when firefox starts.
Should I investigate this?
> @@ +2990,5 @@
> > +    return;
> > +  }
> > +
> > +  if (controllingRegistration) {
> > +    StopControllingADocument(controllingRegistration);
> 
> Good one! Filed https://github.com/slightlyoff/ServiceWorker/issues/682
> since the spec does not actually require this yet. Could you add a test case
> for the situation i've outlined there. the test will go something like this:
> 1) just before registering '/a/b/c', call unregister on '/a'
> 2) after the claim call, calling getRegistration('/a/x/y') should return no
> registration, since /a will now go away
Separate patch coming up.

> @@ +135,5 @@
> > +        resolveClientMessage();
> > +      }
> > +    }
> > +
> > +    return Promise.all([messageFromClient, controllerChangeFromClient])
> 
> I don't understand why controllerChange and messageFromWorker are not tested
> here.

In this case the test window, which is controlled by claim_worker_1, shouldn't be claimed by the second worker and receive either a controllerchange event or a message event. I'm not too fond of this approach because the asserts can only fail intermittently in the event that the second worker manages to claim the test window.
Attachment #8592858 - Attachment is obsolete: true
Blocks: 1145627
Comment on attachment 8601765 [details] [diff] [review]
Implement Service Worker clients.claim.

Review of attachment 8601765 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/ServiceWorkerClients.cpp
@@ +190,5 @@
> +  NS_IMETHOD
> +  Run() override
> +  {
> +    nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();

Don't we need to acquire cleanup lock here before attempting to touch worker private?

@@ +191,5 @@
> +  Run() override
> +  {
> +    nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
> +    MOZ_ASSERT(swm);

move this to right after GetInstance().

@@ +192,5 @@
> +  {
> +    nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> +    WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
> +    MOZ_ASSERT(swm);
> +    MOZ_ASSERT(workerPrivate);

can't assert this until the lock is acquired and IsClean() is false
Attachment #8601765 - Flags: review?(nsm.nikhil) → review+
Attachment #8601765 - Flags: review+
Keywords: leave-open
Hi,

This bug is already landed in m-c. Catalin, does "leave-open" keyword still apply?. Thanks!
Flags: needinfo?(catalin.badea392)
(In reply to Noemí Freire (:noemi) from comment #18)
> Hi,
> 
> This bug is already landed in m-c. Catalin, does "leave-open" keyword still
> apply?. Thanks!

Yes, I need to add 2 more mochitests that were requested in comment 7.
Flags: needinfo?(catalin.badea392)
Status: NEW → ASSIGNED
Component: DOM → DOM: Service Workers
(In reply to Cătălin Badea (:catalinb) from comment #19)
> (In reply to Noemí Freire (:noemi) from comment #18)
> > Hi,
> > 
> > This bug is already landed in m-c. Catalin, does "leave-open" keyword still
> > apply?. Thanks!
> 
> Yes, I need to add 2 more mochitests that were requested in comment 7.

Since there is already patches landed in this bug, and in order to make things less confusing, I have filed the follow-up bug 1175163 for the remaining tests.  Let's close this one.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Depends on: 1175163
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
This has been documented at:

https://developer.mozilla.org/en-US/docs/Web/API/Clients/claim

Also verifying that a note has been added to

https://developer.mozilla.org/en-US/Firefox/Releases/41#Service_Workers

A tech review would be much appreciated. Thanks!
You need to log in before you can comment on or make changes to this bug.