Closed Bug 1239705 Opened 8 years ago Closed 8 years ago

Add a 'Start' button for service workers in about:debugging

Categories

(DevTools :: about:debugging, defect, P1)

defect

Tracking

(firefox47 verified, relnote-firefox 47+)

VERIFIED FIXED
Firefox 47
Tracking Status
firefox47 --- verified
relnote-firefox --- 47+

People

(Reporter: janx, Assigned: janx)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

      No description provided.
Attachment #8707905 - Flags: feedback?(poirot.alex)
This patch adds a 'Start' button to service worker registrations that don't currently have a running worker debugger.

The problem with my approach is that it won't work in e10s, because it should probably run in a content process (furthermore, maybe it should run in the same content process as the website, if it's open?).

Do you know how I could make this work with e10s?
Blocks: 1220747
Comment on attachment 8707905 [details] [diff] [review]
[WIP] Add a 'Start' button for service workers in about:debugging.

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

I got confirmation that service workers are mixed up when using more than one content process (=b2g).
But as soon as we run all tabs in a single child process, it works fine.

Then, we can make various assumptions. But I can't think of a way to make it correct for b2g.
Let's imagine we support only Firefox desktop.
First condition, is e10s enabled?
1) No e10s. Just call attachDebugger (and any other registration method related to worker) in the parent process, where Worker actor should live as there should be no other process. (looks like today's patch)
2) Yes, we have e10s. Then, we know that ServiceWorkerRegistrationActor still lives in parent process, per design of our actors and clients.
But the worker has to live in the child proess if we want the pages to interact with this service worker.
Here, we can tweak today's patch to use message manager to call attachDebugger in the unique child process (that's another condition).

Ideally, the e10s check would be on the server side, in order to support remote debugging of fennec.
With this, we are mostly good. There is just one edge case on desktop: running some tabs in the parent process while e10s is turned on. This will misbehave. But I imagine service workers themselves are also going to have a weird behavior. May be it will be even worse in about:debugging... especially given that will be the first thing you will look into to figure out something being weird around service workers :/
That edge case relates to b2g issue. Given a registration, we can't know in which process we should spawn the worker.
If we somehow can find a set of condition to target the expected process, it would be easy to do somthing with message manager API.

::: devtools/client/aboutdebugging/components/target.js
@@ +73,5 @@
> +    let client = this.props.client;
> +    let target = this.props.target;
> +    if (target.type === "serviceworker" && !target.workerActor) {
> +      client.request({
> +        to: target.actor,

target.actor? Didn't you meant target.workerActor?

::: devtools/server/actors/worker.js
@@ +302,5 @@
>      };
>    },
>  
> +  start: method(function() {
> +    // FIXME This should spawn a WorkerDebugger in a content process.

If you want to spawn it in the child process, you can use message manager API to call attachDebugger on the clone of this registration in the child.

# worker.js:
Services.ppmm.loadProcessScript(
  "resource://devtools/.../service-workers-child.js",
  true);
Services.ppmm.sendAsyncMessage("start", {whateverneeded: ...});

# service-workers-child.js
addMessageListener("start", message => {
  let {data} = message;
  // retrieve the same registration object
  let registration = swm.getSomeHowTheSameOne(data.whateverneeded);
  registration.activeworker.attachDebugger();
});

But you would have to do this only if e10s is turned on.
You can check the related pref on firefox, but that would be great to also support fennec here as they are enable there:
  http://mxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#955

Also, this won't do anything if there is no content process already running. You would need some more code to ensure spawning one before calling sendAsyncMessage.
if (Services.ppmm.childCount == 1) { // = No child process
  let window = Services.appShell.createWindowlessBrowser(true);
  let document = window.document;

  // I don't know if that's really necessary:
  let system = Services.scriptSecurityManager.getSystemPrincipal();
  let interfaceRequestor = window.QueryInterface(Ci.nsIInterfaceRequestor);
  let docShell = interfaceRequestor.getInterface(Ci.nsIDocShell);
  docShell.createAboutBlankContentViewer(system);

  let browser = document.createElementNS(XUL_NS, "browser");
  browser.setAttribute("type", "content");
  browser.setAttribute("remote", "true");
  document.body.appendChild(browser);

  let url = Services.io.newURI("data:text/html,service-workers", null, null);
  let webNav = docShell.QueryInterface(Ci.nsIWebNavigation);
  webNav.loadURI(url, 0, null, null, null);
}

Given the complexity of all this, there is most likely other ways to do this, so feel free to experiment.

@@ +303,5 @@
>    },
>  
> +  start: method(function() {
> +    // FIXME This should spawn a WorkerDebugger in a content process.
> +    this._registration.activeWorker.attachDebugger();

Is `activeWorker` always defined? Even if there is no worker?
Attachment #8707905 - Flags: feedback?(poirot.alex)
I imagine my question is about the main issue of current implementation of service workers...
But let us confirm that's really it:
"Is there a way to know in which process a given registration should ideally be spawning workers into ?"

Here in about:debugging, we are listing registration in the parent process. (If I understood correctly the platform code, registration are replicated in all process, so it is fine listing them only from the parent process?)
And at some point, we would like to spawn a worker for registration that don't have a running worker. But we have to spawn it in the "right process", which I imagine is the main cultprit of current service worker implementation?
Flags: needinfo?(bkelly)
So, we won't really have a good solution to "pick the right process" until we do the work hanging off bug 1231208.

Until then I would use the first child process with the same appID as the service worker's principal.  For e10s desktop that will always be our single child process.  For b2g it will be the corresponding app process, if its running.
Flags: needinfo?(bkelly)
Keywords: dev-doc-needed
Blocks: 1215386
Thank you Alex and Ben for your suggestions!

I'm working on a patch that uses:

    Services.ppmm.addMessageListener("serviceWorkerRegistration:hello", function () {
    });
    Services.ppmm.loadProcessScript(
      "resource://devtools/server/service-worker-child.js", true);
    Services.ppmm.broadcastAsyncMessage("serviceWorkerRegistration:start", {
      scope: this._registration.scope,
      appID: 0 // FIXME
    });

in the registration actor "start" method, and:

    addMessageListener("serviceWorkerRegistration:start", message => {
        Services.ppmm.broadcastAsyncMessage(, {
      let { data } = message;
      // TODO Verify data.appID
      let array = swm.getAllRegistrations();
      for (let i = 0; i < array.length; i++) {
        let registration = array.queryElementAt(index,
          Ci.nsIServiceWorkerRegistrationInfo);
        if (registration.scope === data.scope) {
          registration.activeworker.attachDebugger();
          return;
        }
      }
    });

in a new devtools/server/service-worker-child.js file, but the code in that file never seems to run anywhere (at least my dump()s aren't getting through, and no worker is ever started).
Priority: -- → P1
Has a test and should work fine even in e10s. Alex, could you please have a look?
Attachment #8725333 - Flags: review?(poirot.alex)
Attachment #8707905 - Attachment is obsolete: true
P.S. Could you please push my patch to try as well? I can't currently do that unfortunately.

try: -b do -p linux,linux64,macosx64,win64 -u reftest,reftest-e10s,mochitest-dt,mochitest-e10s-dt -t none
Flags: needinfo?(poirot.alex)
Comment on attachment 8725333 [details] [diff] [review]
Add a 'Start' button for service workers in about:debugging.

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

Looks good overall, but I have some concern about the activeWorker thing.
I still need to test this patch (waiting for firefox to build).

::: devtools/server/actors/worker.js
@@ +5,5 @@
>  const protocol = require("devtools/server/protocol");
>  const { Arg, method, RetVal } = protocol;
>  
>  loader.lazyRequireGetter(this, "ChromeUtils");
> +loader.lazyRequireGetter(this, "Services");

There isn't much value in loading Services lazily as Services.jsm is loaded anyway.

@@ +325,5 @@
>    },
>  
> +  start: method(function() {
> +    Services.ppmm.loadProcessScript(
> +      "resource://devtools/server/service-worker-child.js", true);

This should be called only once. Not each time you press start.
Otherwise the script is going to be evaluated more than once.

::: devtools/server/service-worker-child.js
@@ +13,5 @@
> +  let array = swm.getAllRegistrations();
> +  for (let i = 0; i < array.length; i++) {
> +    let registration =
> +      array.queryElementAt(i, Ci.nsIServiceWorkerRegistrationInfo);
> +    if (registration.scope === data.scope && registration.activeWorker) {

Hum. That looks weird. We want to start the worker. But here we expect "activeWorker" to be available.
You explicitely check if it's defined. In which cases is it undefined?
Aren't we unable to actually start a worker in some cases?

@@ +14,5 @@
> +  for (let i = 0; i < array.length; i++) {
> +    let registration =
> +      array.queryElementAt(i, Ci.nsIServiceWorkerRegistrationInfo);
> +    if (registration.scope === data.scope && registration.activeWorker) {
> +      registration.activeWorker.attachDebugger();

Could you add a comment explaining what attach debugger actually start the worker?
Attachment #8725333 - Flags: review?(poirot.alex)
Thanks for the review and the try push! Treeherder looks happy about my patch.

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> There isn't much value in loading Services lazily as Services.jsm is loaded
> anyway.

Fixed by using `require("Services")` directly.

> > +  start: method(function() {
> > +    Services.ppmm.loadProcessScript(
> > +      "resource://devtools/server/service-worker-child.js", true);
> 
> This should be called only once. Not each time you press start.
> Otherwise the script is going to be evaluated more than once.

I added a boolean attribute `this._processScriptLoaded`, I hope it fixes the problem.

> ::: devtools/server/service-worker-child.js
> @@ +13,5 @@
> > +  let array = swm.getAllRegistrations();
> > +  for (let i = 0; i < array.length; i++) {
> > +    let registration =
> > +      array.queryElementAt(i, Ci.nsIServiceWorkerRegistrationInfo);
> > +    if (registration.scope === data.scope && registration.activeWorker) {
> 
> Hum. That looks weird. We want to start the worker. But here we expect
> "activeWorker" to be available.

"active" here means "the service worker was installed successfully", whether or not it is currently running. There should pretty much always be a `activeWorker` except in some rare corner cases (I added a comment to explain these).

> You explicitely check if it's defined. In which cases is it undefined?
> Aren't we unable to actually start a worker in some cases?

We are unable to actually start a service worker if it didn't finish installing yet, or if there was an uncaught error during install that will cause the registration to be removed. However, I think these cases are very unlikely.

> @@ +14,5 @@
> > +  for (let i = 0; i < array.length; i++) {
> > +    let registration =
> > +      array.queryElementAt(i, Ci.nsIServiceWorkerRegistrationInfo);
> > +    if (registration.scope === data.scope && registration.activeWorker) {
> > +      registration.activeWorker.attachDebugger();
> 
> Could you add a comment explaining what attach debugger actually start the
> worker?

Fixed.
Attachment #8725659 - Flags: review?(poirot.alex)
Attachment #8725333 - Attachment is obsolete: true
Assignee: nobody → janx
Comment on attachment 8725659 [details] [diff] [review]
Add a 'Start' button for service workers in about:debugging.

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

Works great. Still have some important comment to be addressed and possible followups.

Please do address everything. You can keep the races for followups if you want.
And attach a new patch so that I can push to try with a call to detachDebugger.
(I can easily see bad things hapenning here :/)

::: devtools/client/aboutdebugging/components/target.js
@@ +93,5 @@
>    },
>  
> +  start() {
> +    let { client, target } = this.props;
> +    if (target.type === "serviceworker" && !target.workerActor) {

nit: do you think we can somehow share "isRunning" implementation with the one on line 32?

::: devtools/server/actors/worker.js
@@ +329,5 @@
> +    if (!this._processScriptLoaded) {
> +      Services.ppmm.loadProcessScript(
> +        "resource://devtools/server/service-worker-child.js", true);
> +      this._processScriptLoaded = true;
> +    }

This will still execute the script once per registration+start.
You would need a global flag here.

::: devtools/server/service-worker-child.js
@@ +19,5 @@
> +    // XXX: In some rare cases, `registration.activeWorker` can be null for a
> +    // brief moment (e.g. while the service worker is first installing, or if
> +    // there was an unhandled exception during install that will cause the
> +    // registration to be removed). We can't do much about it here, simply
> +    // ignore these cases.

Both are racy?
For the first, you would have to click on Start during worker registration?
The second, you would have to click on Start in between registration and install error removal?
  (It would be interesting to have a test covering registration removal!)

For the first you could possibly listen for activation or some other event to attach a bit later?

@@ +24,5 @@
> +    if (registration.scope === data.scope && registration.activeWorker) {
> +      // Calling `attachDebugger()` on the active service worker forces the
> +      // creation of a worker debugger the current process, effectively
> +      // starting the service worker.
> +      registration.activeWorker.attachDebugger();

You have to call detachDebugger, or the worker will never ever shut down!
Attachment #8725659 - Flags: review?(poirot.alex) → review+
All nits addressed, keeping r+.

Alex, could you please send this patch to try for me?

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> > +  start() {
> > +    let { client, target } = this.props;
> > +    if (target.type === "serviceworker" && !target.workerActor) {
> 
> nit: do you think we can somehow share "isRunning" implementation with the
> one on line 32?

I don't think it's that useful for such a small implementation. Furthermore, `start()` doesn't exactly check for `!isRunning()` here (i.e. it doesn't do anything if `target.type !== "serviceworker"`), which further complicates a shared implementation. I'd prefer to leave it as is for now, and later refactor the status code if needed (e.g. in bug 1153292).

> > +    if (!this._processScriptLoaded) {
> > +      Services.ppmm.loadProcessScript(
> > +        "resource://devtools/server/service-worker-child.js", true);
> > +      this._processScriptLoaded = true;
> > +    }
> 
> This will still execute the script once per registration+start.
> You would need a global flag here.

Fixed by moving the flag up into module scope.

> > +    // XXX: In some rare cases, `registration.activeWorker` can be null for a
> > +    // brief moment (e.g. while the service worker is first installing, or if
> > +    // there was an unhandled exception during install that will cause the
> > +    // registration to be removed). We can't do much about it here, simply
> > +    // ignore these cases.
> 
> Both are racy?
> For the first, you would have to click on Start during worker registration?

More precisely, after the registration is complete, but before the service worker becomes active (once a service worker is successfully registered and downloaded, its code still has to run and go through a few additional steps, i.e. "Install" and "Activate").

> The second, you would have to click on Start in between registration and
> install error removal?
>   (It would be interesting to have a test covering registration removal!)

I believe that's how you would trigger the second race yes. But how we react to this will likely change with bug 1153292, because we'd like to make such errors more user-friendly (e.g. by showing the ghost of a failed registration with some useful details, instead of just removing it).

> For the first you could possibly listen for activation or some other event
> to attach a bit later?

Sounds like a good idea, let's investigate it in a follow-up bug.

> > +      registration.activeWorker.attachDebugger();
> 
> You have to call detachDebugger, or the worker will never ever shut down!

Fixed by adding an immediate call to `detachDebugger()`.
Attachment #8725709 - Flags: review+
Attachment #8725659 - Attachment is obsolete: true
Flags: needinfo?(poirot.alex)
Status: NEW → ASSIGNED
Or Ryan, could you please send this patch to try for me?

try: -b do -p linux,linux64,macosx64,win64 -u reftest,reftest-e10s,mochitest-dt,mochitest-e10s-dt -t none
Flags: needinfo?(poirot.alex) → needinfo?(jryans)
Thank you Ryan! Try looks green to me.
Keywords: checkin-needed
Release Note Request (optional, but appreciated)
[Why is this notable]: Part of new Devtools for Service Workers, adds a button to start registered SWs
[Suggested wording]:
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Ryan, sheriffs seem to be on holiday. Could you please check this patch into fx-team for me?
Flags: needinfo?(jryans)
https://hg.mozilla.org/mozilla-central/rev/0ea808b64514
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
dev-doc-needed: The relevant MDN page is here https://developer.mozilla.org/en-US/docs/Tools/about:debugging
Successfully reproduce this bug on Nightly 46.0a1 (2016-01-14) (Build ID: 20160114030246) on Linux, 64 Bit.

This Bug is now verified as fixed on Latest Firefox Developer Edition 47.0a2 (2016-04-13)

Build ID: 20160413004016
OS: Linux 3.19.0-58-generic x86-64
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
QA Whiteboard: [bugday-20160413]
Reproduced the bug in 46.0a1 (2016-01-14) on windows 10 x64

Verified as fixed with latest Firefox aurora 47.0a2 (Build ID: 20160420004046)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0

As it is also verified on Linux (Comment 24), Marking it as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160413] → [bugday-20160413][bugday-20160420]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: