Closed Bug 982728 Opened 8 years ago Closed 8 years ago

Implement ServiceWorkerGlobalScope unregister()

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: nsm, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

These will be proxies to the main thread equivalents and implemented after ServiceWorkerManager.
Assignee: nobody → amarchesini
Attached patch unregister.patch (obsolete) — Splinter Review
I need a feedback about this: Which scope should I use?
At the moment I used the mScope from the ServiceWorkerGlobalScope but the spec says the prefix of the scope.
Attachment #8479100 - Flags: feedback?(nsm.nikhil)
Comment on attachment 8479100 [details] [diff] [review]
unregister.patch

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

What we discussed about PromiseWorkerProxy.

Other than that, this looks good.

As for scope, using mScope is the way to go. The spec language is confusing, but I think path prefix should be path - if scope is 'http://example.com/foo' then '/foo'.
Of course this is irrelevant to our implementation since we are using absolute URL scopes everywhere.

::: dom/workers/ServiceWorkerManager.cpp
@@ -1038,1 @@
>    nsCOMPtr<nsIGlobalObject> sgo = GetEntryGlobal();

Does this resolve to a sane object since the stack is clean due to being called async?
Attachment #8479100 - Flags: feedback?(nsm.nikhil)
In order to test this new feature I need to post messages from the ServiceWorker to the window. ServiceWorker::postMessage() method is implemented in bug 982726.
Depends on: 982726
OS: Linux → All
Hardware: x86_64 → All
Attached patch unregister.patch (obsolete) — Splinter Review
This doesn't contain a mochitest. It will be included in a separated patch.
Attachment #8479100 - Attachment is obsolete: true
Attachment #8481207 - Flags: review?(nsm.nikhil)
Attachment #8481207 - Flags: review?(nsm.nikhil)
Attached patch unregister.patch (obsolete) — Splinter Review
Attachment #8481207 - Attachment is obsolete: true
Attachment #8481232 - Flags: review?(nsm.nikhil)
Comment on attachment 8481232 [details] [diff] [review]
unregister.patch

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

Dropping review until Andrea verifies that if the worker goes away, resolving the promise does not lead to a crash or other issues. Everything else looks good at a glance.

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

Nit: This seems to be unused.
Attachment #8481232 - Flags: review?(nsm.nikhil)
Attached patch unregister.patch (obsolete) — Splinter Review
Tested locally. We need bug 982726 to be landed in order to write a proper mochitest.
Attachment #8481232 - Attachment is obsolete: true
Attachment #8486414 - Flags: review?(nsm.nikhil)
Comment on attachment 8486414 [details] [diff] [review]
unregister.patch

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

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +31,5 @@
>    nsISupports register(in DOMString aScope, in DOMString aScriptURI);
>  
>    /**
> +   * Unregister an existing ServiceWorker registration for `aScope`.
> +   * It keeps alive the aCallback until the operation is conclused.

keeps aCallback alive until the operation is concluded.

::: dom/workers/ServiceWorkerManager.cpp
@@ +244,5 @@
>  // worker and queues up a runnable to resolve the update promise once the
>  // worker has successfully been parsed.
>  class ServiceWorkerUpdateInstance MOZ_FINAL : public nsISupports
>  {
> +  nsRefPtr<ServiceWorkerRegistrationInfo> mRegistration;

Not part of this patch.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +145,4 @@
>  already_AddRefed<Promise>
>  ServiceWorkerRegistration::Unregister(ErrorResult& aRv)
>  {
> +  nsCOMPtr<nsIGlobalObject> go = GetEntryGlobal();

Just replace all this entry global stuff with GetOwner() now that these checks have moved to registration.

::: dom/workers/WorkerScope.cpp
@@ +13,3 @@
>  #include "mozilla/dom/FunctionBinding.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mozilla/dom/PromiseWorkerProxy.h"

This header is not needed any more.

@@ +409,5 @@
> +  State mState;
> +  bool mValue;
> +};
> +
> +class UnregisterRunnable MOZ_FINAL : public WorkerMainThreadRunnable

We don't need to block the worker while Unregister() is called on the main thread. So I think we can get away with just using a normal nsRunnable here.

@@ +411,5 @@
> +};
> +
> +class UnregisterRunnable MOZ_FINAL : public WorkerMainThreadRunnable
> +                                   , public nsIServiceWorkerUnregisterCallback
> +                                   , public WorkerFeature

While I'm uncomfortable having a runnable be a WorkerFeature and a callback, it doesn't seem unsafe. Please run it by khuey/bent once though to make sure there aren't lifetime edge cases with this sort of setup. Could you add a comment to the top that the lifetime of this runnable is actually controlled by SWM::Unregister() and UnregisterResultRunnable, so that it is acceptable to use it as a WorkerFeature.

@@ +415,5 @@
> +                                   , public WorkerFeature
> +{
> +  nsRefPtr<Promise> mWorkerPromise;
> +  nsString mScope;
> +  bool mCleanUp;

Nit: Call this mCleanedUp.

@@ +450,5 @@
> +  NS_IMETHODIMP
> +  UnregisterSucceeded(bool aState) MOZ_OVERRIDE
> +  {
> +    AssertIsOnMainThread();
> +

This code path can be skipped if mCleanedUp is true. The dispatch will fail anyway, but perhaps it is good to save the allocation when we have the relevant info.

@@ +462,5 @@
> +  NS_IMETHODIMP
> +  UnregisterFailed() MOZ_OVERRIDE
> +  {
> +    AssertIsOnMainThread();
> +

Ditto.

@@ +472,5 @@
> +  }
> +
> +  void
> +  CleanUp(JSContext* aCx)
> +  {

Nit: Worker thread assertion.

@@ +547,5 @@
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  MOZ_ASSERT(mWorkerPrivate->IsServiceWorker());
> +
> +  nsRefPtr<Promise> promise =
> +    Promise::Create(mWorkerPrivate->GlobalScope(), aRv);

Just use |this|?
Attachment #8486414 - Flags: review?(nsm.nikhil) → review+
Attached patch unregister.patch (obsolete) — Splinter Review
another quick glance?
Actually the reason why the RegistrationInfo is nsRefPtr in the runnable is because the registration must be kept alive when the worker unregisters itself.
Attachment #8486414 - Attachment is obsolete: true
Attachment #8486720 - Flags: review?(nsm.nikhil)
Comment on attachment 8486720 [details] [diff] [review]
unregister.patch

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

You haven't incorporated the last 5 suggestions of the previous review. The RefPtr is fine.
Attachment #8486720 - Flags: review?(nsm.nikhil)
> This code path can be skipped if mCleanedUp is true. The dispatch will fail
> anyway, but perhaps it is good to save the allocation when we have the
> relevant info.

I would not do this because otherwise we need to protect mCleanedUp with a mutex. At the moment mCleanedUp is used only in the worker thread.
Attached patch unregister.patchSplinter Review
Attachment #8486720 - Attachment is obsolete: true
Attachment #8486790 - Flags: review?(nsm.nikhil)
Comment on attachment 8486790 [details] [diff] [review]
unregister.patch

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

Could you rename this bug to have only unregister() and file another bug for update().
Attachment #8486790 - Flags: review?(nsm.nikhil) → review+
Summary: Implement ServiceWorkerGlobalScope update() and unregister() → Implement ServiceWorkerGlobalScope unregister()
Blocks: 1065366
Blocks: 1065367
https://hg.mozilla.org/mozilla-central/rev/7266964a7e27
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.