Expose ServiceWorkerRegistration on Workers

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

(Blocks 1 bug, {dev-doc-complete})

33 Branch
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(10 attachments)

25.65 KB, patch
baku
: review+
Details | Diff | Splinter Review
8.58 KB, patch
nsm
: review+
Details | Diff | Splinter Review
7.43 KB, patch
baku
: review+
Details | Diff | Splinter Review
9.09 KB, patch
baku
: review+
Details | Diff | Splinter Review
13.76 KB, patch
baku
: review+
Details | Diff | Splinter Review
17.27 KB, patch
baku
: review+
Details | Diff | Splinter Review
17.42 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.43 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.77 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.86 KB, patch
baku
: review+
Details | Diff | Splinter Review
This includes allowing Workers and SharedWorkers to access registrations via register(), getRegistrations() etc. and allowing the ServiceWorkerGlobalScope to access it's registration.
Does this include intercepting network requests generated from workers?  Or is that a separate bug?  Or does it already work?
Flags: needinfo?(nsm.nikhil)
That is 1032521.
Flags: needinfo?(nsm.nikhil)
This adds the WebIDL changes and introduces relevant C++ classes. The first step is to support update(), unregister() and the access to scope.
Attachment #8591119 - Flags: review?(amarchesini)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
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.
Attachment #8591120 - Flags: review?(amarchesini)
Attachment #8591119 - Attachment is obsolete: true
Attachment #8591119 - Flags: review?(amarchesini)
Attachment #8591120 - Attachment is obsolete: true
Attachment #8591120 - Flags: review?(amarchesini)
ServiceWorkerGlobalScope::Registration() now returns initialized registration.
Attachment #8591133 - Flags: review?(amarchesini)
Attachment #8591129 - Attachment is obsolete: true
Attachment #8591129 - Flags: review?(amarchesini)
Am I using PromiseWorkerProxy correctly?
Attachment #8591137 - Flags: review?(catalin.badea392)
Attachment #8591133 - Attachment is obsolete: true
Attachment #8591133 - Flags: review?(amarchesini)
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.
Attachment #8591140 - Flags: review?(amarchesini)
Attachment #8591137 - Attachment is obsolete: true
Attachment #8591137 - Flags: review?(catalin.badea392)
Attachment #8591140 - Attachment is obsolete: true
Attachment #8591140 - Flags: review?(amarchesini)
Attachment #8591119 - Attachment is obsolete: false
Attachment #8591119 - Flags: review?(amarchesini)
Attachment #8591120 - Attachment is obsolete: false
Attachment #8591120 - Flags: review?(amarchesini)
Attachment #8591129 - Attachment is obsolete: false
Attachment #8591129 - Flags: review?(amarchesini)
Attachment #8591133 - Attachment is obsolete: false
Attachment #8591133 - Flags: review?(amarchesini)
Attachment #8591137 - Attachment is obsolete: false
Attachment #8591137 - Flags: review?(amarchesini)
Attachment #8591140 - Attachment is obsolete: false
Attachment #8591140 - Flags: review?(amarchesini)
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.
Attachment #8591119 - Flags: review?(amarchesini) → review+
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.
Attachment #8591120 - Flags: review?(amarchesini) → review-
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
Attachment #8591129 - Flags: review?(amarchesini) → review+
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 :)
Attachment #8591133 - Flags: review?(amarchesini) → review+
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.
Attachment #8591137 - Flags: review?(amarchesini) → review+
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?
Attachment #8591140 - Flags: review?(amarchesini) → review+
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.
Attachment #8591143 - Flags: review?(amarchesini) → review+
mListener is fully thread-safe, correct?
Comment on attachment 8591120 [details] [diff] [review]
Patch 2 - Expose to workers

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

r+ based on comment 13
Attachment #8591120 - Flags: review- → review+
(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.
Attachment #8592526 - Flags: review?(amarchesini) → review+
Attachment #8594264 - Flags: review?(amarchesini) → review+
Attachment #8594263 - Flags: review?(amarchesini) → review+
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
Depends on: 1157901
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.