Various APIs should return same ServiceWorkerRegistration object with js equality

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bkelly, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
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.
multiple-register.https.html is one.
unregister-then-register.https.html needs to be reverted.
getregistrations.https.html after Bug 1189671 lands.
Ehsan, you were asking about possible bugs to work.  How about this one?  Its a compat thing we need to fix.
Flags: needinfo?(ehsan)
Assignee

Updated

4 years ago
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Assignee

Comment 6

4 years ago
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 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.
Attachment #8690932 - Flags: review?(josh)
Assignee

Comment 9

4 years ago
(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.
Assignee

Updated

4 years ago
Attachment #8690932 - Attachment is obsolete: true
Attachment #8692213 - Flags: review?(josh) → review+

Comment 12

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98fb5254f788
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
See Also: → 1252055
You need to log in before you can comment on or make changes to this bug.