790952 - Debugger server always creates new actor instances

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Mike Hommey [:glandium]]

Preliminary note: I'm looking at code with bug 753401, bug 781515 and bug 789114 applied.

The first thing DSC_onPacket does is to call getActor with the actorID the client sent. getActor may return either an actor "factory" function or an instance. Or at least, that's the intent.

In practice, the first time, getActor returns the "factory" function, creates an instance, and then addActor that instance. The instance, when freshly created, doesn't have an actorID associated with it, so addActor assigns an actorID to the instance before storing it in the pool, but that actorID is different from the original one under which we got the "factory" function.

So the next time DSC_onPacket calls getActor with the actorID the client sent, it still get the "factory" function instead of the instance cached in the pool.

As a side note, if addActor were to end up using the actorID the client sent for the instance, the subsequent removeActor in DSC_onPacket would end up removing it completely from the pool.

Comment 1

[Mike Hommey [:glandium]]

The following works, but is racy:

diff --git a/toolkit/devtools/debugger/server/dbg-server.js b/toolkit/devtools/debugger
--- a/toolkit/devtools/debugger/server/dbg-server.js
+++ b/toolkit/devtools/debugger/server/dbg-server.js
@@ -524,18 +524,19 @@ DebuggerServerConnection.prototype = {
       } catch (e) {
         Cu.reportError(e);
         this.transport.send({
           error: "unknownError",
           message: ("error occurred while creating actor '" + actor.name +
                     "': " + safeErrorString(e))
         });
       }
+      instance.actorID = actor.actorID;
+      actor.registeredPool.removeActor(actor);
       actor.registeredPool.addActor(instance);
-      actor.registeredPool.removeActor(actor);
       actor = instance;
     }
 
     var ret = null;
 
     // Dispatch the request to the actor.
     if (actor.requestTypes && actor.requestTypes[aPacket.type]) {
       try {

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

Your analysis was fantastic and we can even simplify the fix a bit. addActor properly removes any previous instances of actors with the same ID, so just carrying over the ID suffices. The test triggers the bug without this fix, so I'm confident in it.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

Made the test more robust. Try run: https://tbpl.mozilla.org/?tree=Try&rev=d327ff5c0edd

Comment 4

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 662563 [details] [diff] [review]
Patch v2

Good catch, Glandium.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/f0ddac4f8973

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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