ServiceWorkers: Implement [[HandleDocumentUnload]]

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

unspecified
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.

Updated

5 years ago
Assignee: nobody → nicklebedev37
Mentor: nsm.nikhil → ehsan

Comment 2

5 years ago
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?

Updated

5 years ago
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

Comment 5

5 years ago
Posted patch bug-1041340-fix.patch (obsolete) — Splinter Review
Attachment #8492629 - Flags: feedback?(nsm.nikhil)

Updated

5 years ago
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)
https://hg.mozilla.org/mozilla-central/rev/c2bcea8dde26
Status: NEW → RESOLVED
Last Resolved: 4 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.