Closed
Bug 1011268
Opened 11 years ago
Closed 10 years ago
Implement ServiceWorkerContainer unregister()
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
ehsan.akhgari
:
review-
|
Details | Diff | Splinter Review |
(deleted),
text/x-patch
|
Details |
Seperate bug since it has a testing dependency on bug 1002570.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8461195 -
Flags: review?(ehsan)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #8461195 -
Flags: review?(ehsan) → review+
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
Attachment #8462249 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8461195 -
Attachment is obsolete: true
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.
Attachment #8462249 -
Flags: review?(bzbarsky) → review+
Updated•10 years ago
|
Assignee | ||
Comment 6•10 years ago
|
||
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.
Attachment #8476836 -
Flags: review?(ehsan)
Assignee | ||
Updated•10 years ago
|
Attachment #8462249 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
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.
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.
Attachment #8476836 -
Flags: review?(ehsan) → review-
Comment 11•10 years ago
|
||
Also, please add a test case for the same origin check failure case.
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•