We should be able to attach to a service worker.

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 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

4 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

4 years ago
Posted 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+
Assignee

Updated

4 years ago
Depends on: 1220741
Assignee

Updated

4 years ago
No longer depends on: 1219205
Assignee

Updated

4 years ago
No longer depends on: 1220741
Assignee

Updated

4 years ago
Depends on: 1220740
Assignee

Comment 3

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

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

Comment 6

4 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

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