1043004 - Update ServiceWorkerContainer API to spec

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

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

https://github.com/slightlyoff/ServiceWorker/issues/365 changed the webfacing API to make ServiceWorker registrations more intuitive to handle and to seperate the registration from the navigator.serviceWorker.controller instance that stays the same for the lifetime of the document.

This should be a fairly easy bug for contributors with a little bit of prior XPCOM/C++ experience. It involves exposing a ServiceWorkerRegistration WebIDL type, modifying the ServiceWorkerContainer WebIDL and implementation and modifying ServiceWorkerManager to fill in these ServiceWorkerRegistration instances (using useful methods like ServiceWorkerManager::GetRegistration() and ServiceWorkerManager::FindScopeForPath()).

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Spec updated

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PIServiceWorkerRegistration

This patch implements a dummy interface called PIServiceWorkerRegistration that is useful to implement correctly ServiceWorkerManager::AddRegistrationEventListener and ServiceWorkerManager::RemoveRegistrationEventListener

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch 2 - PIServiceWorkerRegistration

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8468395 [details] [diff] [review]
patch 1 - Spec updated

It's not finished yet. sorry.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Spec updated

mochitests were not updated in the previous patch.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8468426 [details] [diff] [review]
patch 2 - PIServiceWorkerRegistration

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

I think this is missing the idl file for ServiceWorkerRegistration

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +6,5 @@
>  #include "domstubs.idl"
>  
>  interface nsIDocument;
>  interface nsIURI;
> +interface PIServiceWorkerRegistration;

The idl for this seems to be missing.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8468426 [details] [diff] [review]
patch 2 - PIServiceWorkerRegistration

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

Also, can we expand the idl interface to define the few methods we need in order to avoid static casting to ServiceWorker?

It appears we just need:

  GetDocumentURI()
  InvalidateWorkerReference()

Also, lets fold this patch into the previous one.  Its a bit confusing doing it as a secondary patch to me.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8468453 [details] [diff] [review]
patch 1 - Spec updated

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

Overall this is looking good, but I'd like to see it again.  In particular, if we could get rid of the static casting in the ServiceWorkerEventListener that would be great.  Overall I think we should be more aggressive with refactoring/cleanup at when we see it at this point.

Thanks!

::: dom/bindings/Bindings.conf
@@ +1077,5 @@
>  
> +'ServiceWorkerRegistration': {
> +    'nativeType': 'mozilla::dom::workers::ServiceWorkerRegistration',
> +    'headerFile': 'mozilla/dom/ServiceWorkerRegistration.h',
> +},

Since this is exposed on Window and not on a worker, should this just be in the mozilla::dom namespace?

If you make that change then I think you can lose the Bindings.conf record altogether.

Also, consider the same changes for ServiceWorkerContainer.

::: dom/webidl/ServiceWorkerContainer.webidl
@@ +17,3 @@
>    [Unforgeable] readonly attribute ServiceWorker? controller;
>  
>    [Throws]

I believe there is a general rule that methods and attributes that return promises should not throw.  I understand this is just for the "not implemented yet" exception, but could we return rejected promises instead?  Would be nice not to accidentally leave the Throws in the final version.

@@ +21,3 @@
>  
>    [Throws]
> +  Promise<ServiceWorkerRegistration> register(DOMString scriptURL, optional RegistrationOptionList options);

scriptURL should be ScalarValueString instead of DOMString.  Also, RegistrationOptionList.scope needs to be fixed to use ScalarValueString as well.

Nit: Can you wrap the second arg to a new line so this fits within 80 chars?

@@ +27,3 @@
>  
>    [Throws]
> +  Promise<any> getRegistrations();

Is this because our webidl does not all Promise<> types to include sequences?  Can you add a comment above this that it should be Promise<Sequence<ServiceWorkerRegistration>> for people looking at the interface?

::: dom/webidl/ServiceWorkerRegistration.webidl
@@ +17,5 @@
> +
> +  readonly attribute ScalarValueString scope;
> +
> +  [Throws]
> +  Promise<any> unregister();

Can we not use Promise<boolean> in our version of webidl?  Can you add a comment indicating the type if not?

Also, can we return a rejected promise and drop [Throws]?

::: dom/workers/ServiceWorkerManager.cpp
@@ +95,5 @@
> +      // certain about this cast.
> +      nsCOMPtr<nsPIDOMWindow> window =
> +        do_QueryInterface(pendingPromise->GetParentObject());
> +      nsRefPtr<ServiceWorkerRegistration> swr =
> +        new ServiceWorkerRegistration(window, NS_ConvertUTF8toUTF16(aScope));

If you make ServiceWorkerRegistration() take an nsISupports instead of a window, then you can pass domObject.GetAsSupports() to the constructor without any QI.  Is there a reason it has to be a window?

Also, what happens to the ServiceWorker we just created above now that we don't return it?  Maybe add a comment about why its created here, how its kept alive, and how it will ultimately be used.  Its a bit mysterious from just looking at the code right now.

@@ +97,5 @@
> +        do_QueryInterface(pendingPromise->GetParentObject());
> +      nsRefPtr<ServiceWorkerRegistration> swr =
> +        new ServiceWorkerRegistration(window, NS_ConvertUTF8toUTF16(aScope));
> +
> +      pendingPromise->MaybeResolve(swr);

What holds the ServiceWorkerRegistration alive if content is not storing the result of this promise?  Is it ok for it to go away immediately?  Or does this need bug 1002571 to work properly?

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +1,2 @@
> +/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
> +/* vim: set ts=2 et sw=2 tw=80: */

Nit: This mode line and others in this patch are out of date with the current preferred one:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

@@ +127,5 @@
> +      rv = swm->GetWaiting(mWindow, getter_AddRefs(serviceWorker));
> +      break;
> +    case WhichServiceWorker::ACTIVE_WORKER:
> +      rv = swm->GetActive(mWindow,
> +                          getter_AddRefs(serviceWorker));

Nit: This doesn't need to be line wrapped.

::: dom/workers/ServiceWorkerRegistration.h
@@ +7,5 @@
> +#ifndef mozilla_dom_workers_ServiceWorkerRegistration_h
> +#define mozilla_dom_workers_ServiceWorkerRegistration_h
> +
> +#include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/workers/ServiceWorkerManager.h"

We're including ServiceWorkerManager.h in the header here just to get access to the WhichServiceWorker enum.  Can we move that enumeration to a separate header which gets included here instead?

@@ +29,5 @@
> +                                           DOMEventTargetHelper)
> +
> +  IMPL_EVENT_HANDLER(updatefound)
> +
> +  ServiceWorkerRegistration(nsPIDOMWindow* aWindow,

Can this and the return value of GetParentObject() just be nsISupports?  May reduce the need for some QI'ing in other code.

@@ +51,5 @@
> +  already_AddRefed<ServiceWorker>
> +  GetActive();
> +
> +  void
> +  GetScope(nsAString& aScope)

This can be const.

@@ +68,5 @@
> +  void
> +  InvalidateWorkerReference(WhichServiceWorker aWhichOnes);
> +
> +private:
> +  ~ServiceWorkerRegistration();

Believe we need a virtual destructor with NS_ISUPPORTS_INHERITED.

@@ +89,5 @@
> +  nsRefPtr<ServiceWorker> mInstallingWorker;
> +  nsRefPtr<ServiceWorker> mWaitingWorker;
> +  nsRefPtr<ServiceWorker> mActiveWorker;
> +
> +  nsString mScope;

This can be const;

@@ +91,5 @@
> +  nsRefPtr<ServiceWorker> mActiveWorker;
> +
> +  nsString mScope;
> +
> +  nsRefPtr<ServiceWorker> mControllerWorker;

This is never referenced and can be removed.

::: dom/workers/test/serviceworkers/test_installation_simple.html
@@ +107,5 @@
>    }
>  
>    // FIXME(nsm): test for parse error when Update step doesn't happen (directly from register).
>  
> +  /* FIXME - re-enable this test when GetRegistration is implemented.

Add bug number please.  I think its bug 1002571.

Comment 9

[Andrea Marchesini [:baku]]

> Also, can we expand the idl interface to define the few methods we need in
> order to avoid static casting to ServiceWorker?

Actually, this pattern is used often and the basic idea is to avoid to expose methods that can be used with a wrong interface. I saw it somewhere else. Actually, something I would like to ask is:
Do we really need nsIServiceWorkerManager? maybe we can get rid of it and use ServiceWorkerManager as a singleton.

> Also, lets fold this patch into the previous one.  Its a bit confusing doing
> it as a secondary patch to me.

Sure!

Comment 10

[Andrea Marchesini [:baku]]

> >    [Unforgeable] readonly attribute ServiceWorker? controller;
> >  
> >    [Throws]
> 
> I believe there is a general rule that methods and attributes that return
> promises should not throw.  I understand this is just for the "not
> implemented yet" exception, but could we return rejected promises instead? 
> Would be nice not to accidentally leave the Throws in the final version.

The creation of a Promise requires a ErrorResult. So that method must throw if something goes wrong creating the promise obj.

> Also, can we return a rejected promise and drop [Throws]?

I cannot.

> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +95,5 @@
> > +      // certain about this cast.
> > +      nsCOMPtr<nsPIDOMWindow> window =
> > +        do_QueryInterface(pendingPromise->GetParentObject());
> > +      nsRefPtr<ServiceWorkerRegistration> swr =
> > +        new ServiceWorkerRegistration(window, NS_ConvertUTF8toUTF16(aScope));
> 
> If you make ServiceWorkerRegistration() take an nsISupports instead of a
> window, then you can pass domObject.GetAsSupports() to the constructor
> without any QI.  Is there a reason it has to be a window?

Yes. ServiceWorkerRegistration uses the window in any methods of ServiceWorkerManager.

> What holds the ServiceWorkerRegistration alive if content is not storing the
> result of this promise?  Is it ok for it to go away immediately?  Or does
> this need bug 1002571 to work properly?

The promise keeps alive the result. ServiceWorkerRegistration is not actually storing any useful information and getRegistration/getRegistrations will return different objects any time they are called.


> Can this and the return value of GetParentObject() just be nsISupports?  May
> reduce the need for some QI'ing in other code.

I should move GetParentObject to the CPP code because I don't want to include "nsIPDOMWindow.h" in the header file.
Then, in general this patter is done everywhere. I don't think that returning nsISupports* changes something.

> @@ +68,5 @@
> > +  void
> > +  InvalidateWorkerReference(WhichServiceWorker aWhichOnes);
> > +
> > +private:
> > +  ~ServiceWorkerRegistration();
> 
> Believe we need a virtual destructor with NS_ISUPPORTS_INHERITED.

ServiceWorkerRegistration is MOZ_FINAL.

Comment 11

[Andrea Marchesini [:baku]]

> Also, can we expand the idl interface to define the few methods we need in
> order to avoid static casting to ServiceWorker?

Actually the main idea of this patch is to avoid the wrong use of AddRegistrationEventListener and RemoveRegistrationEventListener.

What you propose means that domainInfo->mServiceWorkerRegistrations uses PIServiceWorkerRegistration and I don't think it's something we want to do.

I see 2 options:

1. we remove nsIServiceWorkerManager completely - it's actually not used from JS and we can change any
  do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
to
  ServiceWorkerManager::GetInstance()

2. we keep this patch as it is, in order to prevent the use of wrong params in some methods of nsIServiceWorkerManager.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Spec updated

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8472917 [details] [diff] [review]
patch 1 - Spec updated

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

Thanks!  Looks good.  Just a couple nits.  r=me with those addressed.

::: dom/webidl/ServiceWorkerContainer.webidl
@@ +28,4 @@
>  
>    [Throws]
> +  // This should be: Promise<Sequence<ServiceWorkerRegistration>>
> +  Promise<any> getRegistrations();

Are you sure this doesn't work?  I was able to do Promise<sequence<Request>> in my Cache.webidl.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +181,5 @@
> +
> +nsIURI*
> +ServiceWorkerRegistration::GetDocumentURI() const
> +{
> +  return mWindow->GetDocumentURI();

mWindow is checked for non-null above, but used unconditionally here.

Since ServiceWorkerRegistration is only exposed on window, I think we can guarantee we always have a parent.  So lets MOZ_ASSERT(mWindow) in the constructor and remove the mWindow conditionals in the other methods.

If thats not a valid assumption, though, please check for null here before dereferencing.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #11)
> What you propose means that domainInfo->mServiceWorkerRegistrations uses
> PIServiceWorkerRegistration and I don't think it's something we want to do.

Ok, but can you explain why?

> 1. we remove nsIServiceWorkerManager completely - it's actually not used
> from JS and we can change any
>   do_GetService(SERVICEWORKERMANAGER_CONTRACTID, &rv);
> to
>   ServiceWorkerManager::GetInstance()

I like this approach as it would simplify the code, but I'm not familiar with the downsides.

> 2. we keep this patch as it is, in order to prevent the use of wrong params
> in some methods of nsIServiceWorkerManager.

I'm ok with this if there are reasons to keep the XPCOM interface for ServiceWorkerManager.  I just don't understand what its buying us right now as we always static_cast it away.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

#content on IRC suggests that a static GetInstance() is ok and would be faster.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

If you want, we could split that to another bug or just do it on the serviceworker development branch.

Comment 17

[Andrea Marchesini [:baku]]

> If thats not a valid assumption, though, please check for null here before
> dereferencing.

I added MOZ_ASSERT in GetDocumentURI but we still need the check in StopListeningForEvents() because this method is called in the distructor, and, when the object is unlinked, mWindow can be set to nullptr before the distructor is called by SnowWhite.

here a backtrace:

https://tbpl.mozilla.org/php/getParsedLog.php?id=46162209&tree=Try#error0

I'm totally fine to remove nsIServiceWorkerManager in a follow up.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #17)
> > If thats not a valid assumption, though, please check for null here before
> > dereferencing.
> 
> I added MOZ_ASSERT in GetDocumentURI but we still need the check in
> StopListeningForEvents() because this method is called in the distructor,
> and, when the object is unlinked, mWindow can be set to nullptr before the
> distructor is called by SnowWhite.
> 
> here a backtrace:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46162209&tree=Try#error0

Hmm, but you can MOZ_ASSERT(mWindow) in StartListeningForEvents?  Also, can you add a comment in StopListeningForEvents about why it can be null there?  Thanks!

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=56c5d8e1a57e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/261d832d497b

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/261d832d497b

Comment 22

[Chris Mills [:cmills]]

Documented at https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer; updated around March 13th, so I'm pretty sure its up-to-date.

This could do with a tech review. thanks!