1201127 - Various APIs should return same ServiceWorkerRegistration object with js equality

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we mint a new ServiceWorkerRegistration object every time we return one from the various SW APIs.  The recent spec agreement is that these objects should actually be === comparable in js since it helps developers and avoids state synchronization issues.

See:

  https://github.com/slightlyoff/ServiceWorker/issues/416
  bug 1181039 comment 10

Comment 1

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

web platform tests in testing/web-platform/mozilla/tests/service-workers/service-worker and possibly mochitests in dom/workers/test/serviceworkers will need modification. It is easy to find them by grepping for 'equal.*\.scope' or 'registration\.scope' or similar.

Comment 2

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

multiple-register.https.html is one.

Comment 3

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

unregister-then-register.https.html needs to be reverted.

Comment 4

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

getregistrations.https.html after Bug 1189671 lands.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Ehsan, you were asking about possible bugs to work.  How about this one?  Its a compat thing we need to fix.

Comment 6

[(no longer active)]

ServiceWorkerGlobalScope.registration was marked as [SameObject] in bug 1218141, and nothing needs to be done for ServiceWorkerRegistrationWorkerThread because ServiceWorkerContainer is not yet exposed to Workers.

The only change required here is to add a hashtable for ServiceWorkerRegistrationMainThread objects to Window to avoid creating a new object if we have previously created one.

Comment 7

[(no longer active)]

Attachment: Return the same ServiceWorkerRegistration object from service worker APIs dealing with the same underlying registration object

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 8690932 [details] [diff] [review]
Return the same ServiceWorkerRegistration object from service worker APIs dealing with the same underlying registration object

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

::: dom/base/nsPIDOMWindow.h
@@ +863,5 @@
>  
> +  typedef nsRefPtrHashtable<nsStringHashKey,
> +                            mozilla::dom::ServiceWorkerRegistrationMainThread>
> +          ServiceWorkerRegistrationTable;
> +  ServiceWorkerRegistrationTable mServiceWorkerRegistrationTable;

Should we be updating the cycle-collection macros for this?

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +872,5 @@
>  
>    void
> +  RegistrationRemoved() override
> +  {
> +    AssertIsOnMainThread();

I don't get why this is empty.

Comment 9

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #8)
> Comment on attachment 8690932 [details] [diff] [review]
> Return the same ServiceWorkerRegistration object from service worker APIs
> dealing with the same underlying registration object
> 
> Review of attachment 8690932 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsPIDOMWindow.h
> @@ +863,5 @@
> >  
> > +  typedef nsRefPtrHashtable<nsStringHashKey,
> > +                            mozilla::dom::ServiceWorkerRegistrationMainThread>
> > +          ServiceWorkerRegistrationTable;
> > +  ServiceWorkerRegistrationTable mServiceWorkerRegistrationTable;
> 
> Should we be updating the cycle-collection macros for this?

Ah, yes!

> ::: dom/workers/ServiceWorkerRegistration.cpp
> @@ +872,5 @@
> >  
> >    void
> > +  RegistrationRemoved() override
> > +  {
> > +    AssertIsOnMainThread();
> 
> I don't get why this is empty.

It will be filled in bug 1228149.

Comment 10

[(no longer active)]

Attachment: Return the same ServiceWorkerRegistration object from service worker APIs dealing with the same underlying registration object

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98fb5254f788

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98fb5254f788