Closed
Bug 1041340
Opened 10 years ago
Closed 10 years ago
ServiceWorkers: Implement [[HandleDocumentUnload]]
Categories
(Core :: DOM: Workers, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: nsm, Assigned: nsm)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
When a registration stops controlling it's last document, certain things have to be done to modify ServiceWorker state.
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-document-unload-algorithm
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Mentor: nsm.nikhil
Whiteboard: [mentor=nsm.nikhil@gmail.com][lang=c++] → [lang=c++]
Comment 1•10 years ago
|
||
I would like to try this task, i will take a look at the spec more precisely and get back with the questions.
Updated•10 years ago
|
Assignee: nobody → nicklebedev37
Updated•10 years ago
|
Mentor: nsm.nikhil → ehsan
Hello, can I know about the current state of this bug? If no one is currently working on it, may I give it a try?
Assignee | ||
Comment 3•10 years ago
|
||
I can take mentorship back. Thanks ehsan!
Mentor: ehsan → nsm.nikhil
Flags: needinfo?(ehsan)
Comment 4•10 years ago
|
||
Feel free to take the bug since i didn't do any work on it yet.
Sorry for taking and not starting on it.
Flags: needinfo?(nicklebedev37)
Assignee | ||
Updated•10 years ago
|
Assignee: nicklebedev37 → lynn_tran
Attachment #8492629 -
Flags: feedback?(nsm.nikhil)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8492629 [details] [diff] [review]
bug-1041340-fix.patch
Review of attachment 8492629 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/ServiceWorkerManager.cpp
@@ +1048,5 @@
> +ServiceWorkerManager::HandleDocumentUnload(nsIDocument* aDoc)
> +{
> + AssertIsOnMainThread();
> +
> + nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
No need for this.
@@ +1050,5 @@
> + AssertIsOnMainThread();
> +
> + nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
> + nsCOMPtr<nsIURI> documentURI = aDoc->GetDocumentURI();
> + nsRefPtr<ServiceWorkerRegistrationInfo> aRegistration = GetServiceWorkerRegistrationInfo(documentURI);
The registration was already fetched in MaybeStopControlling, pass that to this method. Then you don't need to pass the document.
@@ +1053,5 @@
> + nsCOMPtr<nsIURI> documentURI = aDoc->GetDocumentURI();
> + nsRefPtr<ServiceWorkerRegistrationInfo> aRegistration = GetServiceWorkerRegistrationInfo(documentURI);
> +
> + if (aRegistration->mPendingUninstall) {
> + aRegistration->Clear();
return after this.
@@ +1055,5 @@
> +
> + if (aRegistration->mPendingUninstall) {
> + aRegistration->Clear();
> + }
> + else if (!aRegistration) {
the spec says aRegistration->mWaitingWorker should not be null
@@ +1056,5 @@
> + if (aRegistration->mPendingUninstall) {
> + aRegistration->Clear();
> + }
> + else if (!aRegistration) {
> + swm->FinishActivate(aRegistration);
Activate by dispatching ActivationRunnable to the main thread (see FinishInstall for an example)
@@ +1821,5 @@
> if (!Preferences::GetBool("dom.serviceWorkers.enabled")) {
> return;
> }
>
> + nsRefPtr<ServiceWorkerManager> swm = ServiceWorkerManager::GetInstance();
Already within the service worker manager, so you don't need to GetInstance it.
@@ +1836,5 @@
> if (registration) {
> registration->StopControllingADocument();
> }
> +
> + if (!registration->IsControllingDocuments()) {
Move this block inside the non-null registration check above.
::: dom/workers/ServiceWorkerManager.h
@@ +417,5 @@
> void* aUnused);
>
> nsClassHashtable<nsISupportsHashKey, PendingReadyPromise> mPendingReadyPromises;
> +
> + NS_IMETHODIMP
Return void.
Attachment #8492629 -
Flags: feedback?(nsm.nikhil) → feedback+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 7•10 years ago
|
||
I've landed a patch that uses a tweaked form of Lynn's patch on maple
https://hg.mozilla.org/projects/maple/rev/3dcfe3564f65
Assignee | ||
Updated•10 years ago
|
Mentor: nsm.nikhil
Whiteboard: [lang=c++]
Assignee | ||
Updated•10 years ago
|
Assignee: lynn_tran → nsm.nikhil
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8562856 -
Flags: review?(amarchesini)
Assignee | ||
Updated•10 years ago
|
Attachment #8492629 -
Attachment is obsolete: true
Comment 9•10 years ago
|
||
Comment on attachment 8562856 [details] [diff] [review]
Implement [[HandleDocumentUnload]]
Review of attachment 8562856 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/workers/ServiceWorkerManager.cpp
@@ +1751,5 @@
> + if (!registration->IsControllingDocuments()) {
> + ServiceWorkerJobQueue* queue = GetOrCreateJobQueue(registration->mScope);
> + // The remaining tasks touch registration->mPendingUninstall, so queue
> + // them up in a job.
> + nsRefPtr<ServiceWorkerActivateAfterUnloadingJob> job = new ServiceWorkerActivateAfterUnloadingJob(queue, registration);
80chars
::: dom/workers/ServiceWorkerManager.h
@@ +274,5 @@
> friend class GetRegistrationRunnable;
> friend class QueueFireUpdateFoundRunnable;
> + friend class ServiceWorkerActivateAfterUnloadingJob;
> + friend class ServiceWorkerRegistrationInfo;
> + friend class ServiceWorkerRegisterJob;
alphabetic order.
Attachment #8562856 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
sorry i had to to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=491e81cd9cf8 - seems one of this changes was causing on multiple desktop platforms test failures like
https://treeherder.mozilla.org/logviewer.html#?job_id=6670670&repo=mozilla-inbound
that became frequent to perma failures starting with this csets. Could you take a look at this, thanks!
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(nsm.nikhil)
You need to log in
before you can comment on or make changes to this bug.
Description
•