Ben Kelly [:bkelly, not reviewing] Reporter
	Description • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 3 • 9 years ago

heads up on Bug 1151664.

	Cătălin Badea (:catalinb) Assignee
	Comment 4 • 9 years ago

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.

	Cătălin Badea (:catalinb) Assignee
	Comment 6 • 9 years ago

Fixed a small issue.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 7 • 9 years ago

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.

	José Antonio Olivera Ortega [:jaoo]
	Comment 8 • 9 years ago

/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/

	Cătălin Badea (:catalinb) Assignee
	Comment 9 • 9 years ago

(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?

	José Antonio Olivera Ortega [:jaoo]
	Comment 10 • 9 years ago

(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.

	Cătălin Badea (:catalinb) Assignee
	Comment 12 • 9 years ago

(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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 13 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 14 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=458877295889

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 15 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/65cb9349b849

	Phil Ringnalda (:philor)
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/65cb9349b849

	Noemí Freire (:noemi)
	Comment 18 • 9 years ago

Hi,

This bug is already landed in m-c. Catalin, does "leave-open" keyword still apply?. Thanks!

	Cătălin Badea (:catalinb) Assignee
	Comment 19 • 9 years ago

(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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dcfe0aa8ea2

	Ekanan Ketunuti
	Comment 21 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3dcfe0aa8ea2

	(no longer active)
	Comment 22 • 9 years ago

(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.

	Chris Mills [:cmills]
	Comment 23 • 9 years ago

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!