1131327 - Expose ServiceWorkerRegistration on Workers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

This includes allowing Workers and SharedWorkers to access registrations via register(), getRegistrations() etc. and allowing the ServiceWorkerGlobalScope to access it's registration.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Does this include intercepting network requests generated from workers?  Or is that a separate bug?  Or does it already work?

Comment 2

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

That is 1032521.

Comment 3

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

Attachment: Patch 1 - Introduce ServiceWorkerRegistration{Base,MainThread,WorkerThread}

This adds the WebIDL changes and introduces relevant C++ classes. The first step is to support update(), unregister() and the access to scope.

Comment 4

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

Attachment: Patch 2 - Expose to workers

This patch exposes ServiceWorkerRegistration (and ServiceWorker to satisfy constraints) to workers.

For now, a null registration is returned in the worker.

Please ignore that I'm removing the visibility prefs, I'll add a patch that reintroduces them. It was just easier to develop with them removed.

Comment 5

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

Attachment: Patch 3 - move event listeners to main thread class

Comment 6

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

Attachment: Patch 4 - Implement ServiceWorkerRegistration.update() on worker

ServiceWorkerGlobalScope::Registration() now returns initialized registration.

Comment 7

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

Attachment: Patch 5 - Implement ServiceWorkerRegistration.unregister() on worker

Am I using PromiseWorkerProxy correctly?

Comment 8

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

Attachment: Patch 6 - SWM holds ServiceWorkerRegistrationListener for updatefound and invalidation notifications

Splits up the listener interface to which SWM can hold rawptrs so that registration objects living on the worker thread can use awkward proxies to listen to these events.

Comment 9

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

Attachment: Patch 7 - Fire updatefound on worker registration

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8591119 [details] [diff] [review]
Patch 1 - Introduce ServiceWorkerRegistration{Base,MainThread,WorkerThread}

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +2034,5 @@
>  {
>    AssertIsOnMainThread();
>    nsAutoCString scope = NS_ConvertUTF16toUTF8(aScope);
>  
>    // TODO: this is very very bad:

I guess we can remove this comment.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +33,3 @@
>  NS_INTERFACE_MAP_END_INHERITING(DOMEventTargetHelper)
>  
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerRegistrationBase, DOMEventTargetHelper, mCCDummy);

80chars.

@@ +38,1 @@
>                                                       const nsAString& aScope)

indentation

@@ +180,5 @@
>  }
>  
> +void
> +ServiceWorkerRegistrationMainThread::InvalidateWorkerReference(WhichServiceWorker aWhichOnes)
> +{

AssertIsOnMainTHread();

@@ +243,1 @@
>  {

AssertIsMainThread();

@@ +253,1 @@
>  {

AssertIsMainThread();

@@ +322,5 @@
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(ServiceWorkerRegistrationWorkerThread)
> +NS_INTERFACE_MAP_END_INHERITING(ServiceWorkerRegistrationBase)
> +
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(ServiceWorkerRegistrationWorkerThread, ServiceWorkerRegistrationBase,
> +                                   mCCDummy);

In theory mCCDummy is CCed by the base class. Remove it.

::: dom/workers/ServiceWorkerRegistration.h
@@ +59,5 @@
> +  // DOMEventTargethelper
> +  virtual void DisconnectFromOwner() override;
> +
> +protected:
> +  ~ServiceWorkerRegistrationBase();

virtual

@@ +78,5 @@
> +class ServiceWorkerRegistrationMainThread final : public ServiceWorkerRegistrationBase
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ServiceWorkerRegistrationMainThread, ServiceWorkerRegistrationBase)

80chars max?

@@ +126,5 @@
> +class ServiceWorkerRegistrationWorkerThread final : public ServiceWorkerRegistrationBase
> +{
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(ServiceWorkerRegistrationWorkerThread, ServiceWorkerRegistrationBase)

80 chars

@@ +136,5 @@
> +
> +  void
> +  Update()
> +  {
> +    MOZ_CRASH("FIXME");

next patches I guess.

@@ +165,5 @@
> +private:
> +  ~ServiceWorkerRegistrationWorkerThread()
> +  {}
> +
> +  nsCOMPtr<nsISupports> mCCDummy;

This already exist in the base class.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8591120 [details] [diff] [review]
Patch 2 - Expose to workers

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

I guess you forgot to uncomment something... or it's in another patch.

::: dom/webidl/ServiceWorkerRegistration.webidl
@@ -7,5 @@
>   * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html
>   *
>   */
>  
> -[Pref="dom.serviceWorkers.enabled",

No pref anymore?

::: dom/workers/WorkerScope.cpp
@@ +480,5 @@
> +  //if (!mRegistration) {
> +  //  mRegistration = new ServiceWorkerRegistrationWorkerThread(this);
> +  //}
> +
> +  return mRegistration;

This will crash. because you are returning a nullptr without an ErrorResult.

::: dom/workers/test/serviceworkers/test_serviceworker_interfaces.js
@@ +157,5 @@
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "ServiceWorker",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "ServiceWorkerRegistration",
> +// IMPORTANT: Do not change this list without review from a DOM peer!

alphabetic order. It goes under ServiceWorkerGlobalScope.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8591129 [details] [diff] [review]
Patch 3 - move event listeners to main thread class

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

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +107,5 @@
> +// registered.
> +void
> +ServiceWorkerRegistrationMainThread::StartListeningForEvents()
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(!mListeningForEvents);

::: dom/workers/ServiceWorkerRegistration.h
@@ +59,2 @@
>  protected:
> +  ~ServiceWorkerRegistrationBase()

virtual

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8591133 [details] [diff] [review]
Patch 4 - Implement ServiceWorkerRegistration.update() on worker

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

r+ also to the other patch.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +390,5 @@
> +  MOZ_ASSERT(worker);
> +  worker->AssertIsOnWorkerThread();
> +#endif
> +  nsCOMPtr<nsIRunnable> r = new UpdateRunnable(mScope);
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));

I spoke with smaug about this pattern. He told me that in theory, this is wrong because DispatchToMainThread could fail.
What about if we start doing:

if (NS_FAILED(NS_DispatchToMainThread(r))) {
  NS_WARNING("Failed to dispatch ... ");
}

::: dom/workers/WorkerScope.cpp
@@ +478,5 @@
>  ServiceWorkerGlobalScope::Registration()
>  {
> +  if (!mRegistration) {
> +    mRegistration = new ServiceWorkerRegistrationWorkerThread(mScope);
> +  }

here we are :)

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8591137 [details] [diff] [review]
Patch 5 - Implement ServiceWorkerRegistration.unregister() on worker

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

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +556,5 @@
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<StartUnregisterRunnable> r = new StartUnregisterRunnable(worker, promise, mScope);
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));

see the previous comment.

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8591140 [details] [diff] [review]
Patch 6 - SWM holds ServiceWorkerRegistrationListener for updatefound and invalidation notifications

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +705,1 @@
>                                                                    mRegistration);

If I was bent I would indent this like this:

  nsCOMPtr<nsIRunnable> upr =
    NS_NewRunnableMethodWithArg<ServiceWorkerRegistrationInfo*>(
             swm,
             &ServiceWorkerManager::FireUpdateFoundOnServiceWorkerRegistrations,
             mRegistration);

::: dom/workers/ServiceWorkerRegistration.h
@@ +187,5 @@
>  
>    nsCOMPtr<nsISupports> mCCDummy;
>  };
>  
> +class ServiceWorkerRegistrationProxy final

What's this?

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8591143 [details] [diff] [review]
Patch 7 - Fire updatefound on worker registration

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

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +555,5 @@
> +  void
> +  InvalidateWorkers(WhichServiceWorker aWhichOnes) override
> +  {
> +    AssertIsOnMainThread();
> +    // FIXME(nsm);

next patches?

@@ +763,5 @@
> +ServiceWorkerRegistrationWorkerThread::ReleaseListener(Reason aReason)
> +{
> +  // Can't assert worker thread here since the worker may have gone away.
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  if (mListener) {

if (!mListener) {
  return;
}

@@ +769,5 @@
> +
> +    if (aReason == RegistrationIsGoingAway) {
> +      nsRefPtr<AsyncStopListeningRunnable> r =
> +        new AsyncStopListeningRunnable(mListener);
> +      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));

same comment as the previous patches.

::: dom/workers/ServiceWorkerRegistration.h
@@ +186,5 @@
>  
> +  bool
> +  Notify(JSContext* aCx, workers::Status aStatus) override
> +  {
> +    ReleaseListener(WorkerIsGoingAway);

Do we want to have a boolean to do not call ReleaseListener several times?
This Notify() receives 4 different calls...

::: dom/workers/test/serviceworkers/test_workerupdatefoundevent.html
@@ +4,5 @@
> +-->
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Bug XXXXXXX - Test ServiceWorkerRegistration.onupdatefound on ServiceWorker</title>

Correct bug ID

::: dom/workers/test/serviceworkers/updatefoundevent.html
@@ +1,1 @@
> +<!DOCTYPE html>

same here.

::: dom/workers/test/serviceworkers/worker_updatefoundevent.js
@@ +1,1 @@
> +onactivate = function(e) {

We should add a header with the license.

Comment 17

[Andrea Marchesini [:baku]]

mListener is fully thread-safe, correct?

Comment 18

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

Comment on attachment 8591120 [details] [diff] [review]
Patch 2 - Expose to workers

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

r+ based on comment 13

Comment 19

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

(In reply to Andrea Marchesini (:baku) from comment #16)
> Comment on attachment 8591143 [details] [diff] [review]
> Patch 7 - Fire updatefound on worker registration
> 
> Review of attachment 8591143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerRegistration.cpp
> @@ +555,5 @@
> > +  void
> > +  InvalidateWorkers(WhichServiceWorker aWhichOnes) override
> > +  {
> > +    AssertIsOnMainThread();
> > +    // FIXME(nsm);
> 
> next patches?
> 

This requires ServiceWorker to be exposed to workers, which I'm going to do in another patch. So for now I'm going to have {.installing,.waiting,.active} return null in this patch and follow up.
> 
> ::: dom/workers/ServiceWorkerRegistration.h
> @@ +186,5 @@
> >  
> > +  bool
> > +  Notify(JSContext* aCx, workers::Status aStatus) override
> > +  {
> > +    ReleaseListener(WorkerIsGoingAway);
> 
> Do we want to have a boolean to do not call ReleaseListener several times?
> This Notify() receives 4 different calls...

mListener is the boolean.

Comment 20

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

Attachment: Patch 8 - Return null on worker attribute access.

Comment 21

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

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

Comment 22

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

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

Comment 23

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

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

Comment 24

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

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

Comment 25

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

Attachment: Patch 10 - Remember to remove the feature.

Comment 26

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

Attachment: Patch 9 - Fix windows compiler macro error.

Comment 27

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

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

Comment 28

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/01ebdcc64821
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4972508d3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/09c5b97b9186
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe905b66249
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2774add913
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0295e4472a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e31bd95ab8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e66684151c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b24b2eb809c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc091f913859

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8591143 [details] [diff] [review]
Patch 7 - Fire updatefound on worker registration

>diff --git a/dom/workers/ServiceWorkerRegistration.cpp b/dom/workers/ServiceWorkerRegistration.cpp
>--- a/dom/workers/ServiceWorkerRegistration.cpp
>+++ b/dom/workers/ServiceWorkerRegistration.cpp
>+class WorkerListener final : public ServiceWorkerRegistrationListener
>+{
[...]
>+public:
>+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(WorkerListener)

This was missing an 'override' annotation, which causes clang 3.6 &
newer to spam a "-Winconsistent-missing-override" build warning, which
busts --enable-warnings-as-errors builds (with clang 3.6+).

(This is for AddRef/Release overriding decls for these methods in ServiceWorkerRegistrationListener.)

I pushed a followup to add that keyword, with blanket r+ that ehsan
granted me for fixes of this sort over in bug 1126447 comment 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/5676a6c15258

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01ebdcc64821
https://hg.mozilla.org/mozilla-central/rev/6d4972508d3b
https://hg.mozilla.org/mozilla-central/rev/09c5b97b9186
https://hg.mozilla.org/mozilla-central/rev/3fe905b66249
https://hg.mozilla.org/mozilla-central/rev/3c2774add913
https://hg.mozilla.org/mozilla-central/rev/cc0295e4472a
https://hg.mozilla.org/mozilla-central/rev/5e31bd95ab8c
https://hg.mozilla.org/mozilla-central/rev/40e66684151c
https://hg.mozilla.org/mozilla-central/rev/b24b2eb809c3
https://hg.mozilla.org/mozilla-central/rev/fc091f913859
https://hg.mozilla.org/mozilla-central/rev/5676a6c15258

Comment 31

[Chris Mills [:cmills]]

I've updated relevant ref pages to mention that this functionality is available in workers:

https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/scope
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/installing
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/waiting
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/active
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/update
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/unregister
https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/onupdatefound

https://developer.mozilla.org/en-US/Firefox/Releases/40#Web_Workers
https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers#APIs_available_in_workers

A quick tech review would be great. Thanks