1219255 - We should be able to attach to a service worker.

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Eddy Bruel [:ejpbruel]]

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.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed solution.

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?

Comment 2

[Cătălin Badea (:catalinb)]

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.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: We should be able to attach to a service worker.

Comment 4

[Andrea Marchesini [:baku]]

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.

Comment 5

[Eddy Bruel [:ejpbruel]]

Try push for this patch, with comments by baku addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f039193e4274

Comment 6

[Eddy Bruel [:ejpbruel]]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e72970c5252d

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e72970c5252d