Closed Bug 1221892 Opened 9 years ago Closed 5 years ago

Extend the debugger protocol to get the matching service worker registration.

Categories

(DevTools :: Debugger, enhancement, P5)

enhancement

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1563607

People

(Reporter: ejpbruel, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

As it turns out, if we use the ServiceWorkerManager in the parent process to list service workers, as we do in bug 1218817, we will not be able to attach a debugger to them.

Because service workers actually run in the child process, the object representing the service worker in the parent process doesn't know when a thread for the service worker has already been created in the child process, and will end up creating a second thread for the service worker in the parent process. Although this thread will be debuggable, it will never receive any fetch events, since those happen on the child process.

What we really want is for the object representing the service worker in the parent process to create a thread for the service worker in its child process, but this is far from trivial. If there is no child process, one needs to be created. If there are multiple child processes (as is the case in B2G), it's not obvious how we can tell which child process the service worker live in. And even if we do manage to create the thread in the appropriate child process, it's not obvious if WorkerDebugger is easily remotable to the parent process.

Since fixing this will probably take a lot of work, we're going to take a different approach for the MVP: instead of listing all service workers in about:debugging (which runs in the parent process), we're going to list all service workers that have the same scope as a given tab. Since the debugger for a remote tab runs in a child process, and since any service workers with the same scope as that tab run in the same process, we can always use the ServiceWorkerManager in the tab's process to debug service workers with the same scope.
Once I started working on this, I realised that there can be at most one service worker registration that matches a given tab at any given time. So instead of using a live list, it would make more sense to introduce a matchRegistration request on TabActor that gives you the currently matching registration, and a one-shot notification when the matching registration changes. This is similar to a live list, but somewhat easier to implement, since we're only dealing with at most one actor at any given time.
Summary: Bug 1218817 - Extend the debugger protocol to list all service worker registrations for a given tab. → Extend the debugger protocol to get the matching service worker registration.
Hi Jan. I know this patch is not directly related to about:debugging, but since you already reviewed the patch for bug 1218817, you are probably the best person to review this, since the underlying code is relatively similar. Hope you don't mind!

Flagging vladan for the changes in telemetry.
Attachment #8684207 - Flags: review?(vladan.bugzilla)
Attachment #8684207 - Flags: review?(janx)
Comment on attachment 8684207 [details] [diff] [review]
Bug 1221892 - Extend the debugger protocol to get the matching service worker registration.

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

::: toolkit/components/telemetry/Histograms.json
@@ +6105,5 @@
>      "description": "The time (in milliseconds) that it took a 'listWorkers' request to go round trip."
>    },
> +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_MATCHREGISTRATION_MS": {
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org", "jan@mozilla.com"],
> +    "expires_in_version": "55",

This is 15 months in the future, can you set it to earlier? We can increase the expiry when it gets close to expiring.

@@ +6108,5 @@
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org", "jan@mozilla.com"],
> +    "expires_in_version": "55",
> +    "kind": "exponential",
> +    "high": "10000",
> +    "n_buckets": "50",

Add a "bug_numbers" field (this is a new field):
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Declaring_a_Histogram
Attachment #8684207 - Flags: review?(vladan.bugzilla) → review+
Comment on attachment 8684207 [details] [diff] [review]
Bug 1221892 - Extend the debugger protocol to get the matching service worker registration.

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

(In reply to Eddy Bruel [:ejpbruel] from comment #2)
> Hi Jan. I know this patch is not directly related to about:debugging, but
> since you already reviewed the patch for bug 1218817, you are probably the
> best person to review this, since the underlying code is relatively similar.
> Hope you don't mind!

Hi Eddy! No problem, I'm happy to help with this.

You're going to find this surprising given my earlier review, but now that you've convinced me about using the complete name "serviceWorkerRegistration", I find "matchRegistration" unnecessarily obscure. (What does "match" mean? It sounds like a sort of association between the child process and its service worker registration(s?) that's hard to grasp). If there is really only one service worker registration per tab at any given time, please call your files and methods something with "serviceWorkerRegistration". The fact that we "match" URLs is an implementation detail, and the singular indicates that there can be at most one serviceWorkerRegistration associated to a given tab.

::: devtools/client/debugger/test/mochitest/browser_dbg_matchregistration.js
@@ +25,5 @@
> +
> +      info("Register a service worker in the same scope as the page, and " +
> +           "check that it becomes the matching registration.");
> +      executeSoon(() => {
> +        evalInTab(tab, "promise1 = navigator.serviceWorker.register('" + 

Nit: Trailing space.

::: devtools/client/debugger/test/mochitest/head.js
@@ +1075,5 @@
>      });
>    });
>  }
>  
> +function matchRegistration(tabClient) {

Nit: getServiceWorkerRegistration?

@@ +1113,5 @@
>      });
>    });
>  }
>  
> +function waitForMatchingRegistrationChanged(tabClient) {

Nit: waitForServiceWorkerRegistrationChanged?

::: devtools/server/actors/webbrowser.js
@@ +126,5 @@
> + * The service worker specification defines the service worker registration that
> + * matches a given client URL as the registration which scope is the longest
> + * prefix of the client URL (see section 9.15 for details).
> + */
> +function matchRegistration(clientURL) {

Nit: getServiceWorkerRegistration?

@@ +138,5 @@
> +      continue;
> +    }
> +
> +    if (matchingRegistration === null ||
> +        matchingRegistration.scope.length < registration.scope.length) {

I'm not sure I understand your algorithm: Are you really trying to find the service worker registration whose scope is the longest available substring of clientURL? Can there be multiple registrations with scopes like "domain.com", "sub.domain.com", "longer-sub.domain.com", "domain.com/section/page", etc? Is there only one that "matches" the given clientURL?

@@ +1169,5 @@
> +   * a one-shot notification when the matching service worker registration
> +   * changes, provided the client has sent this request at least once since the
> +   * last notification was sent.
> +   */
> +  onMatchRegistration: function () {

Nit: onGetServiceWorkerRegistration?

@@ +1184,5 @@
> +    if (this._matchingRegistration === registration) {
> +      // The previously matching registration equals the currently matching
> +      // registration, so reuse the actor for the previously matching
> +      // registration.
> +      actor = this._matchingRegistrationActor;  

Nit: Trailing spaces.

@@ +1195,5 @@
> +      this.conn.removeActorPool(this._matchingRegistrationActorPool);
> +
> +      // Create a new actor pool for the currently matching registration.
> +      let pool = new ActorPool(this.conn);
> +      this.conn.addActorPool(pool);

Given that there will always be at most one actor for this, maybe it's not necessary to create a pool for it? Couldn't you use `this.conn.{add,remove}Actor(actor)` directly? Or use the common, persistent actor pool `this._tabActorPool`, if that makes sense?

@@ +1197,5 @@
> +      // Create a new actor pool for the currently matching registration.
> +      let pool = new ActorPool(this.conn);
> +      this.conn.addActorPool(pool);
> +
> +      // If there is no service worker registration tht matches the URL of the

Nit: s/tht/that/.

@@ +1230,5 @@
> +    }
> +  },
> +
> +  _notifyMatchingRegistrationChanged: function () {
> +    this.conn.sendActorEvent(this.actorID, "matchingRegistrationChanged");

Nit: "serviceWorkerRegistrationChanged"?

@@ +1967,5 @@
>    "reconfigure": TabActor.prototype.onReconfigure,
>    "switchToFrame": TabActor.prototype.onSwitchToFrame,
>    "listFrames": TabActor.prototype.onListFrames,
> +  "listWorkers": TabActor.prototype.onListWorkers,
> +  "matchRegistration": TabActor.prototype.onMatchRegistration

Nit: "getServiceWorkerRegistration"?

::: devtools/server/actors/worker.js
@@ +220,5 @@
>  };
>  
>  exports.WorkerActorList = WorkerActorList;
> +
> +function RegistrationActor(registration) {

Nit: ServiceWorkerRegistrationActor?

@@ +224,5 @@
> +function RegistrationActor(registration) {
> +  this._registration = registration;
> +}
> +
> +RegistrationActor.prototype = {

I guess you will extend this in the future with a method to list all associated serviceWorkerActors?

::: devtools/shared/client/main.js
@@ +159,5 @@
>    "tabListChanged": "tabListChanged",
>    "reflowActivity": "reflowActivity",
>    "addonListChanged": "addonListChanged",
>    "workerListChanged": "workerListChanged",
> +  "matchingRegistrationChanged": "matchingRegistrationChanged",

Nit: "serviceWorkerRegistrationChanged"?

@@ +1337,5 @@
>      telemetry: "LISTWORKERS"
>    }),
>  
> +  matchRegistration: DebuggerClient.requester({
> +    type: "matchRegistration",

Nit: "getServiceWorkerRegistration"?

::: toolkit/components/telemetry/Histograms.json
@@ +6104,5 @@
>      "n_buckets": "50",
>      "description": "The time (in milliseconds) that it took a 'listWorkers' request to go round trip."
>    },
> +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_MATCHREGISTRATION_MS": {
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org", "jan@mozilla.com"],

How about "ejpbruel@mozilla.com" instead of "jan@mozilla.com"? ;)

@@ +6112,5 @@
> +    "n_buckets": "50",
> +    "description": "The time (in milliseconds) that it took a 'matchRegistration' request to go round trip."
> +  },
> +  "DEVTOOLS_DEBUGGER_RDP_REMOTE_MATCHREGISTRATION_MS": {
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org", "jan@mozilla.com"],

Same as above.
Attachment #8684207 - Flags: review?(janx) → feedback+
(In reply to Jan Keromnes [:janx] from comment #4)
> Comment on attachment 8684207 [details] [diff] [review]
> Bug 1221892 - Extend the debugger protocol to get the matching service
> worker registration.
> 
> Review of attachment 8684207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Eddy Bruel [:ejpbruel] from comment #2)
> > Hi Jan. I know this patch is not directly related to about:debugging, but
> > since you already reviewed the patch for bug 1218817, you are probably the
> > best person to review this, since the underlying code is relatively similar.
> > Hope you don't mind!
> 
> Hi Eddy! No problem, I'm happy to help with this.
> 
> You're going to find this surprising given my earlier review, but now that
> you've convinced me about using the complete name
> "serviceWorkerRegistration", I find "matchRegistration" unnecessarily
> obscure. (What does "match" mean? It sounds like a sort of association
> between the child process and its service worker registration(s?) that's
> hard to grasp). If there is really only one service worker registration per
> tab at any given time, please call your files and methods something with
> "serviceWorkerRegistration". The fact that we "match" URLs is an
> implementation detail, and the singular indicates that there can be at most
> one serviceWorkerRegistration associated to a given tab.
> 

Told you that shortening this would make things obscure ;-)

The request name is based on an algorithm (match service worker registration) that is defined by the service worker specification. I wrote some comments to clarify this, but apparently the intent is still not clear. Do you have any suggestions on how I could improve on this?

> ::: devtools/client/debugger/test/mochitest/browser_dbg_matchregistration.js
> @@ +25,5 @@
> > +
> > +      info("Register a service worker in the same scope as the page, and " +
> > +           "check that it becomes the matching registration.");
> > +      executeSoon(() => {
> > +        evalInTab(tab, "promise1 = navigator.serviceWorker.register('" + 
> 
> Nit: Trailing space.
> 
> ::: devtools/client/debugger/test/mochitest/head.js
> @@ +1075,5 @@
> >      });
> >    });
> >  }
> >  
> > +function matchRegistration(tabClient) {
> 
> Nit: getServiceWorkerRegistration?
> 
> @@ +1113,5 @@
> >      });
> >    });
> >  }
> >  
> > +function waitForMatchingRegistrationChanged(tabClient) {
> 
> Nit: waitForServiceWorkerRegistrationChanged?
> 
> ::: devtools/server/actors/webbrowser.js
> @@ +126,5 @@
> > + * The service worker specification defines the service worker registration that
> > + * matches a given client URL as the registration which scope is the longest
> > + * prefix of the client URL (see section 9.15 for details).
> > + */
> > +function matchRegistration(clientURL) {
> 
> Nit: getServiceWorkerRegistration?
> 
> @@ +138,5 @@
> > +      continue;
> > +    }
> > +
> > +    if (matchingRegistration === null ||
> > +        matchingRegistration.scope.length < registration.scope.length) {
> 
> I'm not sure I understand your algorithm: Are you really trying to find the
> service worker registration whose scope is the longest available substring
> of clientURL? Can there be multiple registrations with scopes like
> "domain.com", "sub.domain.com", "longer-sub.domain.com",
> "domain.com/section/page", etc? Is there only one that "matches" the given
> clientURL?

Yes. That is how the service worker specification defines the matching service worker registration. I double checked this with Anne van Kesteren to be sure.

> 
> @@ +1169,5 @@
> > +   * a one-shot notification when the matching service worker registration
> > +   * changes, provided the client has sent this request at least once since the
> > +   * last notification was sent.
> > +   */
> > +  onMatchRegistration: function () {
> 
> Nit: onGetServiceWorkerRegistration?
> 
> @@ +1184,5 @@
> > +    if (this._matchingRegistration === registration) {
> > +      // The previously matching registration equals the currently matching
> > +      // registration, so reuse the actor for the previously matching
> > +      // registration.
> > +      actor = this._matchingRegistrationActor;  
> 
> Nit: Trailing spaces.
> 
> @@ +1195,5 @@
> > +      this.conn.removeActorPool(this._matchingRegistrationActorPool);
> > +
> > +      // Create a new actor pool for the currently matching registration.
> > +      let pool = new ActorPool(this.conn);
> > +      this.conn.addActorPool(pool);
> 
> Given that there will always be at most one actor for this, maybe it's not
> necessary to create a pool for it? Couldn't you use
> `this.conn.{add,remove}Actor(actor)` directly? Or use the common, persistent
> actor pool `this._tabActorPool`, if that makes sense?
> 
> @@ +1197,5 @@
> > +      // Create a new actor pool for the currently matching registration.
> > +      let pool = new ActorPool(this.conn);
> > +      this.conn.addActorPool(pool);
> > +
> > +      // If there is no service worker registration tht matches the URL of the
> 
> Nit: s/tht/that/.
> 
> @@ +1230,5 @@
> > +    }
> > +  },
> > +
> > +  _notifyMatchingRegistrationChanged: function () {
> > +    this.conn.sendActorEvent(this.actorID, "matchingRegistrationChanged");
> 
> Nit: "serviceWorkerRegistrationChanged"?
> 
> @@ +1967,5 @@
> >    "reconfigure": TabActor.prototype.onReconfigure,
> >    "switchToFrame": TabActor.prototype.onSwitchToFrame,
> >    "listFrames": TabActor.prototype.onListFrames,
> > +  "listWorkers": TabActor.prototype.onListWorkers,
> > +  "matchRegistration": TabActor.prototype.onMatchRegistration
> 
> Nit: "getServiceWorkerRegistration"?
> 
> ::: devtools/server/actors/worker.js
> @@ +220,5 @@
> >  };
> >  
> >  exports.WorkerActorList = WorkerActorList;
> > +
> > +function RegistrationActor(registration) {
> 
> Nit: ServiceWorkerRegistrationActor?
> 
> @@ +224,5 @@
> > +function RegistrationActor(registration) {
> > +  this._registration = registration;
> > +}
> > +
> > +RegistrationActor.prototype = {
> 
> I guess you will extend this in the future with a method to list all
> associated serviceWorkerActors?
> 

Jup!

> ::: devtools/shared/client/main.js
> @@ +159,5 @@
> >    "tabListChanged": "tabListChanged",
> >    "reflowActivity": "reflowActivity",
> >    "addonListChanged": "addonListChanged",
> >    "workerListChanged": "workerListChanged",
> > +  "matchingRegistrationChanged": "matchingRegistrationChanged",
> 
> Nit: "serviceWorkerRegistrationChanged"?
> 
> @@ +1337,5 @@
> >      telemetry: "LISTWORKERS"
> >    }),
> >  
> > +  matchRegistration: DebuggerClient.requester({
> > +    type: "matchRegistration",
> 
> Nit: "getServiceWorkerRegistration"?
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +6104,5 @@
> >      "n_buckets": "50",
> >      "description": "The time (in milliseconds) that it took a 'listWorkers' request to go round trip."
> >    },
> > +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_MATCHREGISTRATION_MS": {
> > +    "alert_emails": ["dev-developer-tools@lists.mozilla.org", "jan@mozilla.com"],
> 
> How about "ejpbruel@mozilla.com" instead of "jan@mozilla.com"? ;)
> 
> @@ +6112,5 @@
> > +    "n_buckets": "50",
> > +    "description": "The time (in milliseconds) that it took a 'matchRegistration' request to go round trip."
> > +  },
> > +  "DEVTOOLS_DEBUGGER_RDP_REMOTE_MATCHREGISTRATION_MS": {
> > +    "alert_emails": ["dev-developer-tools@lists.mozilla.org", "jan@mozilla.com"],
> 
> Same as above.

My bad :-)
New patch with comments by Jan addressed.
Attachment #8684800 - Flags: review?(janx)
Comment on attachment 8684800 [details] [diff] [review]
Extend the debugger protocol to get the matching service worker registration.

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

(In reply to Eddy Bruel [:ejpbruel] from comment #5)
> Told you that shortening this would make things obscure ;-)
> 
> The request name is based on an algorithm (match service worker
> registration) that is defined by the service worker specification. I wrote
> some comments to clarify this, but apparently the intent is still not clear.
> Do you have any suggestions on how I could improve on this?

I'm not sure that all these methods should be named after an algorithm from the service workers spec.

What the new methods in your patch are trying to achieve is to get a unique serviceWorkerRegistration for a given tabClient, and I'm not sure that users of this API will care about the underlying "matching" algorithm.

In short, maybe drop the word "match" from your more public method and event names? And maybe use "get" instead of "match" if you need a verb. I'll suggest such changes in my new nits, but I don't feel too strongly about this, so if you feel that it's clearer to keep the word "match" somewhere, I'll stop complaining about it.

> > ::: devtools/client/debugger/test/mochitest/browser_dbg_matchregistration.js
> > @@ +25,5 @@
> > > +
> > > +      info("Register a service worker in the same scope as the page, and " +
> > > +           "check that it becomes the matching registration.");
> > > +      executeSoon(() => {
> > > +        evalInTab(tab, "promise1 = navigator.serviceWorker.register('" + 
> > 
> > Nit: Trailing space.

You forgot to address this.

> > I'm not sure I understand your algorithm: Are you really trying to find the
> > service worker registration whose scope is the longest available substring
> > of clientURL? Can there be multiple registrations with scopes like
> > "domain.com", "sub.domain.com", "longer-sub.domain.com",
> > "domain.com/section/page", etc? Is there only one that "matches" the given
> > clientURL?
> 
> Yes. That is how the service worker specification defines the matching
> service worker registration. I double checked this with Anne van Kesteren to
> be sure.

Ah, thanks for the details. "longer-sub.domain.com" vs "domain.com/page" still looks like a conflicting case to me, but maybe I should look at the specification.

> > @@ +1195,5 @@
> > > +      this.conn.removeActorPool(this._matchingRegistrationActorPool);
> > > +
> > > +      // Create a new actor pool for the currently matching registration.
> > > +      let pool = new ActorPool(this.conn);
> > > +      this.conn.addActorPool(pool);
> > 
> > Given that there will always be at most one actor for this, maybe it's not
> > necessary to create a pool for it? Couldn't you use
> > `this.conn.{add,remove}Actor(actor)` directly? Or use the common, persistent
> > actor pool `this._tabActorPool`, if that makes sense?

You forgot to address this.

::: devtools/client/debugger/test/mochitest/head.js
@@ +1075,5 @@
>      });
>    });
>  }
>  
> +function matchServiceWorkerRegistration(tabClient) {

Nit: getServiceWorkerRegistration(tabClient)?

@@ +1078,5 @@
>  
> +function matchServiceWorkerRegistration(tabClient) {
> +  info("Matching service worker registration.");
> +  return new Promise(function (resolve) {
> +    tabClient.matchServiceWorkerRegistration(function (response) {

Nit: tabClient.getServiceWorkerRegistration()?

@@ +1113,5 @@
>      });
>    });
>  }
>  
> +function waitForMatchingServiceWorkerRegistrationChanged(tabClient) {

Nit: waitForServiceWorkerRegistrationChanged(tabClient)? (without the "Matching")

@@ +1117,5 @@
> +function waitForMatchingServiceWorkerRegistrationChanged(tabClient) {
> +  info("Waiting for matching service worker registration to change.");
> +  return new Promise(function (resolve) {
> +    tabClient.addListener("matchingServiceWorkerRegistrationChanged", function listener() {
> +      tabClient.removeListener("matchingServiceWorkerRegistrationChanged", listener);

Nit: "serviceWorkerRegistrationChanged"? (without the "matching")

::: devtools/server/actors/webbrowser.js
@@ +1214,5 @@
> +      // well as the registration itself. On subsequent requests, this will
> +      // become the previously matching registration.
> +      this._matchingServiceWorkerRegistration = registration;
> +      this._matchingServiceWorkerRegistrationActorPool = pool;
> +      this._matchingServiceWorkerRegistrationActor = actor;

Nit: this._serviceWorkerRegistration{,Actor}?

@@ +1969,5 @@
>    "reconfigure": TabActor.prototype.onReconfigure,
>    "switchToFrame": TabActor.prototype.onSwitchToFrame,
>    "listFrames": TabActor.prototype.onListFrames,
> +  "listWorkers": TabActor.prototype.onListWorkers,
> +  "matchServiceWorkerRegistration": TabActor.prototype.onMatchServiceWorkerRegistration

Nit: "getServiceWorkerRegistration"? (just because "get" is shorter and easier to understand than "match", but maybe you feel that we should use "match" here to call out the underlying algorithm?)

::: devtools/shared/client/main.js
@@ +159,5 @@
>    "tabListChanged": "tabListChanged",
>    "reflowActivity": "reflowActivity",
>    "addonListChanged": "addonListChanged",
>    "workerListChanged": "workerListChanged",
> +  "matchingServiceWorkerRegistrationChanged": "matchingServiceWorkerRegistrationChanged",

Nit: "serviceWorkerRegistrationChanged"? (maybe it's enough to say that the unique registration for a tab has changed, without specifying that that registration is determined by the "match registration" algorithm?)

@@ +1337,5 @@
>      telemetry: "LISTWORKERS"
>    }),
>  
> +  matchServiceWorkerRegistration: DebuggerClient.requester({
> +    type: "matchServiceWorkerRegistration",

Nit: As above, maybe "getServiceWorkerRegistration"?

::: toolkit/components/telemetry/Histograms.json
@@ +6124,5 @@
> +    "expires_in_version": "50",
> +    "kind": "exponential",
> +    "high": "10000",
> +    "n_buckets": "50",
> +    "description": "The time (in milliseconds) that it took a 'matchServiceWorkerRegistration' request to go round trip."

Vladan suggested using the new "bug_numbers" field.

Here, it would look like this: `"bug_numbers": [1221892]` (usually just before the "description" field).

@@ +6126,5 @@
> +    "high": "10000",
> +    "n_buckets": "50",
> +    "description": "The time (in milliseconds) that it took a 'matchServiceWorkerRegistration' request to go round trip."
> +  },
> +  "DEVTOOLS_DEBUGGER_RDP_REMOTE_MATCHREGISTRATION_MS": {

This histogram ID is outdated and breaks the mochitests.

I was able to make your test work by just fixing this.

@@ +6132,5 @@
> +    "expires_in_version": "50",
> +    "kind": "exponential",
> +    "high": "10000",
> +    "n_buckets": "50",
> +    "description": "The time (in milliseconds) that it took a 'matchServiceWorkerRegistration' request to go round trip."

Same as above.
Attachment #8684800 - Flags: review?(janx) → feedback+
New patch with comments addressed.

I no longer using a separate actor pool for the registration actor. Note that this does depend on bug 1223766, which makes sure that actors are destroyed when they are removed from an actor pool. I also no longer separately store the previous registration, since that can be obtained from the actor for the previous registration.
Attachment #8684207 - Attachment is obsolete: true
Attachment #8684800 - Attachment is obsolete: true
Attachment #8686050 - Flags: review?(janx)
Depends on: 1223766
Comment on attachment 8686050 [details] [diff] [review]
Extend the debugger protocol to get the matching service worker registration.

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

Eddy, thanks a lot for addressing all my nits!

I believe you left two dead references to `this._serviceWorkerRegistration`, but apart from that everything looks great, we're very close to r+. For your next review request, could you please push a try run?

And again, sorry for the many back-and-forths and uninteresting nits.

::: devtools/server/actors/webbrowser.js
@@ +1172,5 @@
> +   */
> +  onGetServiceWorkerRegistration: function () {
> +    // The actor for the current service worker registration. This will
> +    // initially be set to null. Whenever possible, we will reuse the actor for
> +    // the previousl service worker registration. Otherwise, a new actor for the

Nit: s/previousl/previous/.

@@ +1189,5 @@
> +         this._serviceWorkerRegistrationActor.registration === registration) ||
> +        (this._serviceWorkerRegistrationActor === null &&
> +         registration === null)) {
> +      // The previous service worker registration equals the current service
> +      // worker registration, so reuse the actor for the previousl service

Nit: previousl.

@@ +1199,5 @@
> +      // service worker registration, so remove the actor for the previous
> +      // service worker registration from the tab actor pool (this will cause
> +      // the actor to be destroyed).
> +      if (this._serviceWorkerRegistrationActor) {
> +        this._tabPool.removeActor(this._serviceWorkerRegistrationActor);

Just curious, why are you using `this._tabPool.{add,remove}Actor()` instead of `this.conn.{add,remove}Actor()`?

@@ +1239,5 @@
> +    this._mustNotifyServiceWorkerRegistrationChanged = false;
> +  },
> +
> +  onRegister: function () {
> +    if (this._serviceWorkerRegistration !== matchServiceWorkerRegistration(this.url)) {

I don't think you cache this anymore. Maybe `this._serviceWorkerRegistrationActor && (this._serviceWorkerRegistrationActor.registration !== match(this.url))` ?

@@ +1245,5 @@
> +    }
> +  },
> +
> +  onUnregister: function () {
> +    if (this._serviceWorkerRegistration !== matchServiceWorkerRegistration(this.url)) {

Same as above.
Attachment #8686050 - Flags: review?(janx) → feedback+
New patch with comments addressed. I use the tab pool because the lifetime of this particular service worker registration is scoped to the tab, so when you detach from the tab, I feel the registration actor for the tab should be destroyed too.
Attachment #8686050 - Attachment is obsolete: true
Attachment #8686465 - Flags: review?(janx)
Comment on attachment 8686465 [details] [diff] [review]
Extend the debugger protocol to get the matching service worker registration.

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

Thanks Eddy! All good with me, don't forget to format your patch with "r=janx p=vladan" at the end of the description.

Please fix my last-minutes nits before landing, or include them as drive-by fixes in your next patch.

(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> I use the tab pool because the lifetime
> of this particular service worker registration is scoped to the tab, so when
> you detach from the tab, I feel the registration actor for the tab should be
> destroyed too.

I'm not quite sure what this will mean for about:debugging when we add service worker registration support, but I guess that this makes sense for your current use-case (since detaching from a tab means that your toolbox closes, you don't care if registrations live longer than a tab).

But given the time constraint around your patch queue, and the fact that we don't really know yet, let's land this patch (which shouldn't break any existing features) and figure out if we need to change the lifetime later.

Maybe you could add a comment like "// FIXME (Bug 1212797) Is tabPool the right lifetime for this?" above the `this._tabPool.addActor` line?

::: devtools/server/actors/webbrowser.js
@@ +1184,5 @@
> +    // Note that we can obtain the previous service worker registration from the
> +    // actor for the previous service worker registration. If no such actor
> +    // exists, the previous service registration was null.
> +    let registration = matchServiceWorkerRegistration(this.url);
> +    if ((this._serviceWorkerRegistrationActor === null &&

Nit: What about a `let previous = this._serviceWorkerRegistrationActor` shortcut variable to make all this more readable?

@@ +1245,5 @@
> +    if ((this._serviceWorkerRegistrationActor === null &&
> +         registration !== null) ||
> +        (this._serviceWorkerRegistrationActor !== null &&
> +         this._serviceWorkerRegistrationActor.registration !== registration)) {
> +      this._notifyServiceWorkerRegistrationChanged();

Nit: The logic is is good but hard to read. Please create a shortcut variable: `let actor = this._serviceWorkerRegistrationActor`.

@@ +1255,5 @@
> +    if ((this._serviceWorkerRegistrationActor === null &&
> +         registration !== null) ||
> +        (this._serviceWorkerRegistrationActor !== null &&
> +         this._serviceWorkerRegistrationActor.registration !== registration)) {
> +      this._notifyServiceWorkerRegistrationChanged();

Nit: Same as above, please create an `actor` shortcut variable.

::: devtools/server/actors/worker.js
@@ +229,5 @@
> +  get registration() {
> +    return this._registration;
> +  },
> +
> +  actorPrefix: "serviceWorkerRegistration",

Nit: We generally place the `actorPrefix` member at the top of the prototype.
Attachment #8686465 - Flags: review?(janx) → review+
Seeing some test failures on try, but those seem to be related to bug 1220970.
Jan, I have an idea on how to simplify the logic for this. I will address this in bug 1220741.
Brian, Nick suggested that the crash I'm seeing in comment 16 could be caused by some of your recent work on object unboxing. The test in question was already failing intermittently, my patch just made the failure permanent (except on OS X). Any idea what could cause this?

This is blocking service worker debugging, for which we are on a tight deadline at the moment, so if you could respond asap, I'd greatly appreciate it.
Flags: needinfo?(ejpbruel) → needinfo?(bhackett1024)
(In reply to Eddy Bruel [:ejpbruel] from comment #17)
> Brian, Nick suggested that the crash I'm seeing in comment 16 could be
> caused by some of your recent work on object unboxing. The test in question
> was already failing intermittently, my patch just made the failure permanent
> (except on OS X). Any idea what could cause this?
> 
> This is blocking service worker debugging, for which we are on a tight
> deadline at the moment, so if you could respond asap, I'd greatly appreciate
> it.

Has the test been failing intermittently in this same way?  That isEmpty() assertion means there is a compartment problem: a compartment is destroyed while one of its ObjectGroups is still alive.  The isEmpty() assertion has been hit before in bug 1181908, but that case was fixed by the patch in bug 1181908 comment 26 and I haven't seen anything about the assert being hit since then.
Flags: needinfo?(bhackett1024)
(In reply to Brian Hackett (:bhackett) from comment #18)
> (In reply to Eddy Bruel [:ejpbruel] from comment #17)
> > Brian, Nick suggested that the crash I'm seeing in comment 16 could be
> > caused by some of your recent work on object unboxing. The test in question
> > was already failing intermittently, my patch just made the failure permanent
> > (except on OS X). Any idea what could cause this?
> > 
> > This is blocking service worker debugging, for which we are on a tight
> > deadline at the moment, so if you could respond asap, I'd greatly appreciate
> > it.
> 
> Has the test been failing intermittently in this same way?  That isEmpty()
> assertion means there is a compartment problem: a compartment is destroyed
> while one of its ObjectGroups is still alive.  The isEmpty() assertion has
> been hit before in bug 1181908, but that case was fixed by the patch in bug
> 1181908 comment 26 and I haven't seen anything about the assert being hit
> since then.

Bug 1220741 contains the following crash log:
https://treeherder.mozilla.org/logviewer.html#?job_id=16690193&repo=mozilla-inbound

It doesn't seem to hit the same assertion, but I also can't really decipher what *is* causing the crash in that case.
(In reply to Eddy Bruel [:ejpbruel] from comment #19)
> Bug 1220741 contains the following crash log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=16690193&repo=mozilla-
> inbound
> 
> It doesn't seem to hit the same assertion, but I also can't really decipher
> what *is* causing the crash in that case.

It looks like that crash is inside AddressSanitizer itself (I've never seen a crash like that before though either).
(In reply to Eddy Bruel [:ejpbruel] from comment #19)
> It doesn't seem to hit the same assertion, but I also can't really decipher
> what *is* causing the crash in that case.

There are a number of intermittents like that on file, that happen rarely, so I wouldn't worry about it if it isn't happening much: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=AddressSanitizer%20CHECK%20failed&list_id=12684481
(In reply to Andrew McCreight [:mccr8] from comment #21)
> (In reply to Eddy Bruel [:ejpbruel] from comment #19)
> > It doesn't seem to hit the same assertion, but I also can't really decipher
> > what *is* causing the crash in that case.
> 
> There are a number of intermittents like that on file, that happen rarely,
> so I wouldn't worry about it if it isn't happening much:
> https://bugzilla.mozilla.org/buglist.
> cgi?quicksearch=AddressSanitizer%20CHECK%20failed&list_id=12684481

Well, the problem is that my patch seems to turn that intermittent test failure into a permanent one, so it got backed out. At first glance, the intermittent failure doesn't seem to be quite the same as the permanent one, though. Note the test is not touching any of the code in the patch, so it doesn't look like I'm causing the problem directly.

At the moment, I can't even reproduce this failure locally, so I'm unsure how to proceed.
I'll do some try pushes to see if we can get closer to the root of the problem.
(In reply to Brian Hackett (:bhackett) from comment #23)
> I'll do some try pushes to see if we can get closer to the root of the
> problem.

Thanks Brian! I appreciate your help on this!

I should note that we agreed during the service worker planning debugging meeting that we should try to fix this test, but if it takes us too long to figure out (say, longer than a couple of days), we're going to disable it instead, and make re-enabling it a priority.
(In reply to Brian Hackett (:bhackett) from comment #23)
> I'll do some try pushes to see if we can get closer to the root of the
> problem.

Hi Brian. Did you figure anything out from your try patches? As it turns out, bug 1218817 also causes this test to crash.
Flags: needinfo?(bhackett1024)
(In reply to Eddy Bruel [:ejpbruel] from comment #25)
> Hi Brian. Did you figure anything out from your try patches? As it turns
> out, bug 1218817 also causes this test to crash.

Well, I did a try push (https://hg.mozilla.org/try/rev/f7a987b30148) that shows that a compartment is being destroyed while one of its ObjectGroups is marked.  I looked at how compartments are marked and currently marking an ObjectGroup does not ensure the compartment itself is marked.  This shouldn't be a problem because we don't keep strong references on ObjectGroups inside the engine except from objects and a couple other things that ensure the compartment is marked, but it is brittle.  I tried fixing this (https://hg.mozilla.org/try/rev/195f772d349f) but the problem still occurs.  It seems like a GC thing to me.
Flags: needinfo?(bhackett1024) → needinfo?(terrence)
Compartment death is the most baffling part of our already quite baffling GC. They are not rooted or held directly live by anything, they just evaporate when their global gets collected. But there's a ton of random crap that can hold them live "accidentally" that we special case. I think :billm is the only one who has ever completely understood how this all works, however, it's been years since he's worked on any of this.

Have you been able to get this to happen in rr? Is it perma-fail locally as well?
Flags: needinfo?(terrence)
OK, I guess given that situation it would be better if the JSCompartment destructor did not assume that all ObjectGroups inside it have already been swept.  This patch fixes the !isEmpty crash on the unboxed layouts list.  With this patch, however, the devtools mochitest still crashes at !isEmpty, but on a *different* mozilla::LinkedList --- gcWeakMapList in the Zone destructor.  Which would (again) suggest some sort of compartment / tracing problem in the test independent of unboxed objects, and which would also help explain why the unboxed layouts list crash doesn't show up in the wild or during fuzzing.
Attachment #8690452 - Flags: review?(terrence)
Comment on attachment 8690452 [details] [diff] [review]
unlink unboxed layouts on compartment death

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

I am strongly against taking this: it is not the root cause and there are even more assertions behind the ones that this patch uncovers. 

The compartment and zone destruction assertions are there for a reason: all objects can (and do) use their compartment and zone pointers, so destroying our compartment or zone while there are still live objects with a pointer to them will inevitably cause us to crash if anything at all touches those objects again, up to and including the finalizer. Something in the original patch is causing tracing inside of a dead compartment. I'll take a closer look at the patch, but I doubt I'll see anything as I'm not familiar with any of these interfaces. The best strategy for finding the failure is still going to be rr, or failing that, bisecting it manually on try.
Attachment #8690452 - Flags: review?(terrence) → review-
Depends on: 1227641
Depends on: 1228046
No longer blocks: 1212797
Not part of Push DevTools for DevEdition 47 anymore.
No longer blocks: 1214248
Last month we landed leak reporting for the JS heap that can be used in conjunction with the CC's leak logging to easily track down this sort of problem. I think we did track this down the hard way already though. Is there more to do here?
(In reply to Terrence Cole [:terrence] from comment #31)
> Last month we landed leak reporting for the JS heap that can be used in
> conjunction with the CC's leak logging to easily track down this sort of
> problem.

Is there documentation about this somewhere?  Or if not, how do you use it?

Even if it's not needed here, it sounds like it could help with other leaks, so I'd love to know more!
Flags: needinfo?(terrence)
The GC just always prints out every leaked thing to stdout. If you are dumping GC and CC logs as per [1], then you can search for the paths in those logs that are holding the printed objects live during shutdown. Hopefully, this will pinpoint whatever is holding the edges live that should have already been cleared during shutdown.

1- https://developer.mozilla.org/en-US/docs/Mozilla/Performance/GC_and_CC_logs#Generating_GC_and_CC_logs
Flags: needinfo?(terrence)
Assignee: ejpbruel → nobody
Product: Firefox → DevTools
Blocks: dbg-worker
Type: defect → enhancement
Priority: -- → P5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: