Closed Bug 1219255 Opened 5 years ago Closed 5 years ago

We should be able to attach to a service worker.

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(1 file, 1 obsolete file)

For service worker debugging, we want to introduce the notion of attaching to a service worker. When a client attaches to a service worker, an underlying worker is created if one does not exist already, and this worker is guaranteed to stay alive as long as there are still clients attached.

This will allow us to always fire up a debugger in a service worker, even when there currently are no events to be processed, and will guarantee that the debugger will not be destroyed when the service worker goes to sleep.
Attached patch Proposed solution. (obsolete) — Splinter Review
The basic idea is that we spawn a worker if needed when the first client attaches, and then disable the idle timeout until the last client detaches. This should make sure that there is always an underlying worker as long as there is at least one client attached.

I'm flagging the patch for feedback rather than review mainly because I'm not 100% certain if this is the right approach, or whether there are any corner cases that I overlooked. Does this patch make sense to you?
Attachment #8680022 - Flags: feedback?(catalin.badea392)
Comment on attachment 8680022 [details] [diff] [review]
Proposed solution.

Review of attachment 8680022 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! One thing that might break this is NoteStoppedControllingDocuments(), but we should remove that.

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp?from=ServiceWorkerPrivate.cpp#1442

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1296,5 @@
>    TerminateWorker();
>  }
>  
> +nsresult
> +ServiceWorkerPrivate::Attach()

Maybe the naming could be more explicit. AttachDebugger or something like that.
Attachment #8680022 - Flags: feedback?(catalin.badea392) → feedback+
Depends on: 1220741
No longer depends on: 1219205
No longer depends on: 1220741
Depends on: 1220740
Attachment #8680022 - Attachment is obsolete: true
Attachment #8688259 - Flags: review?(amarchesini)
Comment on attachment 8688259 [details] [diff] [review]
We should be able to attach to a service worker.

Review of attachment 8688259 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +26,1 @@
>  [scriptable, builtinclass, uuid(1a1e71dd-0f78-4e2e-a2db-a946fe02cddf)]

change this UUID.

::: dom/workers/ServiceWorkerManager.cpp
@@ +4472,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +ServiceWorkerInfo::GetDebugger(nsIWorkerDebugger **aResult)

nsIWorkerDebugger** aResult

@@ +4473,5 @@
>  }
>  
> +NS_IMETHODIMP
> +ServiceWorkerInfo::GetDebugger(nsIWorkerDebugger **aResult)
> +{

if (NS_WARN_IF(!aResult)) {
  return NS_ERROR_FAILURE;
}

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1459,5 @@
>    TerminateWorker();
>  }
>  
> +nsresult
> +ServiceWorkerPrivate::GetDebugger(nsIWorkerDebugger **aResult)

nsIWorkerDebugger** aResult

@@ +1461,5 @@
>  
> +nsresult
> +ServiceWorkerPrivate::GetDebugger(nsIWorkerDebugger **aResult)
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(aResult);

@@ +1500,5 @@
> +ServiceWorkerPrivate::DetachDebugger()
> +{
> +  AssertIsOnMainThread();
> +
> +  if (!mDebuggerCount) {

I would assert here.

@@ +1511,5 @@
> +  // timeout, or terminate the worker if there are no more active tokens.
> +  if (!mDebuggerCount) {
> +    if (mTokenCount) {
> +      ResetIdleTimeout();
> +    }

} else {

::: dom/workers/ServiceWorkerPrivate.h
@@ +128,5 @@
>    void
>    NoteStoppedControllingDocuments();
>  
> +  nsresult
> +  GetDebugger(nsIWorkerDebugger **aResult);

nsIWorkerDebugger**

::: dom/workers/test/serviceworkers/test_serviceworkerinfo.xul
@@ +58,5 @@
> +
> +          SimpleTest.finish();
> +        });
> +      }); 
> +    }

extra space.
Attachment #8688259 - Flags: review?(amarchesini) → review+
Try push for this patch, with comments by baku addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f039193e4274
The try push shows test failures, which are probably due to not properly cleaning up in the test. Here's a second try push with that problem hopefully resolved:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbaf4edec6d2
https://hg.mozilla.org/mozilla-central/rev/e72970c5252d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.