Closed Bug 1011268 Opened 8 years ago Closed 8 years ago

Implement ServiceWorkerContainer unregister()

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nsm, Assigned: nsm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Seperate bug since it has a testing dependency on bug 1002570.
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment on attachment 8461195 [details] [diff] [review]
Unregister algorithm

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

r=me with the below fixed.

::: dom/workers/ServiceWorkerManager.cpp
@@ +461,5 @@
> +    nsRefPtr<ServiceWorkerManager::ServiceWorkerDomainInfo> domainInfo =
> +      swm->GetDomainInfo(mScopeURI);
> +    MOZ_ASSERT(domainInfo);
> +
> +    if (!domainInfo) {

I convinced myself that GetDomainInfo can never return null.  Please remove this block, and file a follow-up to explicitly check to make sure we can get the host from the URL during registration so that we can both sleep better.

@@ +469,5 @@
> +
> +    nsCString spec;
> +    nsresult rv = mScopeURI->GetSpecIgnoringRef(spec);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      mPromise->MaybeReject(rv);

Please reject to undefined.

@@ +476,5 @@
> +
> +    nsRefPtr<ServiceWorkerRegistration> registration;
> +    if (!domainInfo->mServiceWorkerRegistrations.Get(spec,
> +                                                     getter_AddRefs(registration))) {
> +      mPromise->MaybeResolve(JS::UndefinedHandleValue);

Don't we want to reject the promise in case we don't have a registration?  Can you please raise a spec issue?

@@ +498,5 @@
> +    }
> +
> +    if (registration->mWaitingWorker) {
> +      // FIXME(nsm): Set state to redundant.
> +      // Fire statechange.

Please add bug#'s to these FIXME's before landing.

@@ +503,5 @@
> +      registration->mWaitingWorker = nullptr;
> +    }
> +
> +    mPromise->MaybeResolve(JS::UndefinedHandleValue);
> +

Please add a FIXME + bug# for:

11. Wait until no document is using registration as their Service Worker registration.

@@ +726,5 @@
> +  rv = documentPrincipal->CheckMayLoad(scopeURI, true /* report */,
> +                                       false /* allowIfInheritsPrinciple */);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_DOM_SECURITY_ERR;
> +  }

Please ask Boris for review on the origin checks.

::: dom/workers/test/serviceworkers/test_unregister.html
@@ +36,5 @@
> +
> +    var w;
> +    setTimeout(function() {
> +      w = window.open("unregister/index.html", "_blank", "width=700, height=400");
> +    }, 150);

Watch me work against my principles and not r- because of these.  :(  Please add some comments here explaining why the timeouts, and the bug# which will take them out.

::: dom/workers/test/serviceworkers/unregister/index.html
@@ +14,5 @@
> +<div id="content" style="display: none"></div>
> +<pre id="test"></pre>
> +<script class="testbody" type="text/javascript">
> +  function fail(msg) {
> +      info("unregister/index.html: " + msg);

Four space indentation is the devil himself.

@@ +27,5 @@
> +      if (!e.data) {
> +          return fail("Message had no data!");
> +      }
> +
> +      // FIXME(nsm): Use controller and not current!

Bug# please.
Attachment #8461195 - Flags: review?(ehsan) → review+
Attached patch Unregister algorithm (obsolete) — Splinter Review
Boris, could you take a look at if my use of CheckMayLoad in ServiceWorkerManager::Unregister() satisfies the requirements of the spec - 
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#unregister-algorithm
Attachment #8462249 - Flags: review?(bzbarsky)
Comment on attachment 8462249 [details] [diff] [review]
Unregister algorithm

This doesn't seem to be using the "entry settings object's API base URL" unless I'm missing something.

The error reporting CheckMayLoad will do is presumably not the right thing for here, right?

In the !GetExtantDoc() case, why not just bail out immediately?  A nullprincipal will never pass a CheckMayLoad check.

Past that, CheckMayLoad is probably the right thing here, yeah.
Attachment #8462249 - Flags: review?(bzbarsky) → review+
Blocks: ServiceWorkers
No longer blocks: 984048
Modified to use Entry Document and fixed bz's comments. I've removed the window parameter since we just use the entry global object and document. Ehsan, could you just give it a quick once over. Attaching interdiff.
Attachment #8476836 - Flags: review?(ehsan)
Comment on attachment 8476837 [details]
interdiff

The interdiff only has the parameter removal. The entry document changes are in the full patch and I do not have an interdiff for it. Sorry.
Comment on attachment 8476836 [details] [diff] [review]
Unregister algorithm

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

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +14,5 @@
>    // Returns a Promise
>    nsISupports register(in nsIDOMWindow aWindow, in DOMString aScope, in DOMString aScriptURI);
>  
>    // Returns a Promise
> +  nsISupports unregister(in DOMString aScope);

rev the uuid please.

::: dom/workers/ServiceWorkerManager.cpp
@@ +535,5 @@
> +
> +      registration->Clear();
> +      domainInfo->RemoveRegistration(registration);
> +    }
> +      

Nit: trailing whitespace is the devil.  It doesn't look like it, but that's the trick that the devil pulls to crawl into our code base.

@@ +930,5 @@
> +  nsCOMPtr<nsIPrincipal> documentPrincipal = document->NodePrincipal();
> +  rv = documentPrincipal->CheckMayLoad(scopeURI, true /* report */,
> +                                       false /* allowIfInheritsPrinciple */);
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_DOM_SECURITY_ERR;

These SecurityError's should be used to reject the promise, and that should happen asynchronously.
Attachment #8476836 - Flags: review?(ehsan) → review-
Also, please add a test case for the same origin check failure case.
https://hg.mozilla.org/mozilla-central/rev/af9b8e19bb4a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.