1025077 - Implement ServiceWorkerContainer.ready

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: ready.patch

I don't know if we can optimize the way we resolve the pending entry promises.
Maybe you have a better idea.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8477332 [details] [diff] [review]
ready.patch

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

This patch does not deal with the active worker changing after a Promise has already been vended to a ServiceWorkerContainer. I think you'll need to keep a reference to the ServiceWorkerContainers like before to make that work :(
Please also add a test case for the above case with the new patch (if it isn't possible to test due to API limitations like lack of replace(), file a followup)

Could you also file a followup to see if we can get away with ServiceWorkerManager only keeping references to ServiceWorkerContainers and the Containers dealing with ServiceWorkerRegistration IDL objects? ServiceWorkerManager will only deal with ServiceWorkerRegistrationInfos, not with ServiceWorkerRegistrationInfos.

I haven't looked at the rest of the code closely after spotting the issue above, but it looks good cursorily.

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +12,4 @@
>  interface nsIServiceWorkerManager : nsISupports
>  {
>    // Returns a Promise
>    nsISupports register(in nsIDOMWindow aWindow, in DOMString aScope, in DOMString aScriptURI);

This part will conflict with a patch i landed for Bug 1057135, so you will have to upload a new patch for checkin (or just push to m-i yourself). Thanks!

::: dom/workers/ServiceWorkerContainer.cpp
@@ +141,5 @@
>    MOZ_ASSERT(ret);
>    return ret.forget();
>  }
>  
> +Promise*

Why is this a rawptr instead of creating a copy of the refptr and forgetting it?
If it's a valid reason, please add a comment.

::: dom/workers/ServiceWorkerManager.cpp
@@ +753,5 @@
> +    nsCOMPtr<nsIURI> docURI = doc->GetDocumentURI();
> +    if (!docURI) {
> +      mPromise->MaybeReject(NS_ERROR_UNEXPECTED);
> +      return NS_OK;
> +    }

The above checks can be moved into ::GetReadyPromise.

Comment 3

[Andrea Marchesini [:baku]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #2)
> Comment on attachment 8477332 [details] [diff] [review]
> ready.patch
> 
> Review of attachment 8477332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch does not deal with the active worker changing after a Promise has
> already been vended to a ServiceWorkerContainer. I think you'll need to keep

No and I didn't find this in the spec. 'ready' is an attribute, is not a method.
When the promise is sent to the Container and it's resolved, we cannot change its value.
Is this what you meant?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I should've linked to the spec issue https://github.com/slightlyoff/ServiceWorker/issues/223
See @coonsta's last comment.

The thing is it deals with fairly edge case-y things, and it is very wonky behaviour from an attribute (sigh!). I have added a comment to the issue. I guess meanwhile we can stick with this and file followups. I'll do a full review right now.

Comment 5

[Andrea Marchesini [:baku]]

> The thing is it deals with fairly edge case-y things, and it is very wonky
> behaviour from an attribute (sigh!).

I think 'ready' is misleading.
To me 'ready' should be a boolean. 'whenReady' is a better name because it makes sense that is a promise. But again, it should return a boolean.

What we want here is a callback that is executed any time a ServiceWorker is active. A promise is not good for this because it has just 1 resolve function.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8477332 [details] [diff] [review]
ready.patch

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

The rest of it looks good, but please answer the rawptr questions above.

::: dom/workers/ServiceWorkerContainer.cpp
@@ +29,5 @@
>  NS_IMPL_ADDREF_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper)
>  NS_IMPL_RELEASE_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper)
>  
>  NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper,
> +                                   mControllerWorker, mReadyPromise)

Shouldn't mWindow also be in here since we hold a COMPtr? I might be wrong.

::: dom/workers/ServiceWorkerManager.cpp
@@ +802,5 @@
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
> +  if (!window) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

Can you add an assertion mPendingReadyPromises contains window.

::: dom/workers/test/serviceworkers/test_installation_simple.html
@@ +150,5 @@
> +    document.body.appendChild(frame);
> +
> +    var channel = new MessageChannel();
> +    frame.addEventListener('load', function() {
> +      frame.contentWindow.postMessage('your port!', '*', [channel.port2]);

very anthropocentric ;)

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrea Marchesini (:baku) from comment #5)
> > The thing is it deals with fairly edge case-y things, and it is very wonky
> > behaviour from an attribute (sigh!).
> 
> I think 'ready' is misleading.
> To me 'ready' should be a boolean. 'whenReady' is a better name because it
> makes sense that is a promise. But again, it should return a boolean.
> 
> What we want here is a callback that is executed any time a ServiceWorker is
> active. A promise is not good for this because it has just 1 resolve
> function.

Can you put this in the spec issue?

Comment 8

[Andrea Marchesini [:baku]]

> The rest of it looks good, but please answer the rawptr questions above.

Because mReadyPromise is an internal value already refcounted.
For instance, GetConsole does the same, but GetLocation does not in WorkerScope...
I think there is no need to create an extra object just to return an already_AddRefed for an internal value.

I asked roc an opinion:

<baku> do we have any preference between returning it with:
<baku> Something* fnc() { return mSomething; }
<baku> or 
<baku> already_AddRefed<Something> fnc() { nsRefPtr<Something> a = mSomething; return a.forget(); }
<baku> ?
<roc> the former

> >  NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerContainer, DOMEventTargetHelper,
> > +                                   mControllerWorker, mReadyPromise)

Correct. We should use GetOwner() maybe... follow up?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: ready.patch

Actually we don't need to hold a reference to mWindow.
This patch uses GetOwner() everywhere and overwrite DisconnectFromOwner() to unregister ServiceWorkerContainer from the ServiceWorkerManager.

The reason why we need the window is because the Promise and the ServiceWorkerRegistration objects need the window in the constructor.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8478273 [details] [diff] [review]
ready.patch

Does RemoveReadyPromise do anything interesting with the Window object?
I hope it doesn't for example take a strong ref, since 
DisconnectFromOwner can be called when the Window is about to be deleted.

But assuming RemoveReadyPromise just removes, fine.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: ready.patch

https://tbpl.mozilla.org/?tree=Try&rev=42b606e75e62

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: ready.patch

Comment 14

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e48a1782de89

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e48a1782de89

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrea, does this have the same problem as 1058043? Could you fix it?

Comment 17

[Andrea Marchesini [:baku]]

I uploaded a patch on bug 1058043. It's in your review queue.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: container.patch

This patch has been reviewed by nsm in Bug 1058043
We decided to move this patch here because it was more related to this bug than the other one.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aed61458371b

Comment 19

[Andrea Marchesini [:baku]]

Bug reopened.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aed61458371b

Comment 21

[Chris Mills [:cmills]]

See https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer/ready for the doc on this.