Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Description • 10 years ago

Seperate bug since it has a testing dependency on bug 1002570.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 2 • 10 years ago

https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/3e505e3ac2b2

	(no longer active)
	Comment 3 • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 4 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 6 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=4e66b65a725a

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 7 • 10 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 9 • 10 years ago

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.

	(no longer active)
	Comment 10 • 10 years ago

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.

	(no longer active)
	Comment 11 • 10 years ago

Also, please add a test case for the same origin check failure case.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 12 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=da31c0de3adb

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Assignee
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/af9b8e19bb4a

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/af9b8e19bb4a