Closed Bug 1041340 Opened 10 years ago Closed 10 years ago

ServiceWorkers: Implement [[HandleDocumentUnload]]

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: nsm, Assigned: nsm)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Depends on: 984048
Whiteboard: [mentor=nsm.nikhil@gmail.com][lang=c++]
Mentor: nsm.nikhil
Whiteboard: [mentor=nsm.nikhil@gmail.com][lang=c++] → [lang=c++]
I would like to try this task, i will take a look at the spec more precisely and get back with the questions.
Assignee: nobody → nicklebedev37
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?
Flags: needinfo?(nicklebedev37)
Flags: needinfo?(ehsan)
I can take mentorship back. Thanks ehsan!
Mentor: ehsan → nsm.nikhil
Flags: needinfo?(ehsan)
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: nicklebedev37 → lynn_tran
Attached patch bug-1041340-fix.patch (obsolete) (deleted) — Splinter Review
Attachment #8492629 - Flags: feedback?(nsm.nikhil)
Flags: needinfo?(nsm.nikhil)
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+
Flags: needinfo?(nsm.nikhil)
I've landed a patch that uses a tweaked form of Lynn's patch on maple https://hg.mozilla.org/projects/maple/rev/3dcfe3564f65
Mentor: nsm.nikhil
Whiteboard: [lang=c++]
Assignee: lynn_tran → nsm.nikhil
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+
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)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Flags: needinfo?(nsm.nikhil)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: