We should be able to attach to a service worker.

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8680022 [details] [diff] [review]
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?
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+
(Assignee)

Updated

3 years ago
Depends on: 1220741
(Assignee)

Updated

3 years ago
No longer depends on: 1219205
(Assignee)

Updated

3 years ago
No longer depends on: 1220741
(Assignee)

Updated

3 years ago
Depends on: 1220740
(Assignee)

Comment 3

3 years ago
Created attachment 8688259 [details] [diff] [review]
We should be able to attach to a service worker.
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+
(Assignee)

Comment 5

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

Comment 6

3 years ago
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 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e72970c5252d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.