Closed Bug 790952 Opened 7 years ago Closed 7 years ago

Debugger server always creates new actor instances

Categories

(DevTools :: Debugger, defect, P1)

14 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: glandium, Assigned: past)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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 {
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → past
Status: NEW → ASSIGNED
Attachment #660873 - Flags: feedback?(mh+mozilla)
Attachment #660873 - Flags: feedback?(mh+mozilla) → feedback+
Attachment #660873 - Flags: review?(rcampbell)
No longer blocks: 753401, 781515, 789114
Depends on: 753401, 781515, 789114
Attached patch Patch v2Splinter Review
Made the test more robust. Try run: https://tbpl.mozilla.org/?tree=Try&rev=d327ff5c0edd
Attachment #660873 - Attachment is obsolete: true
Attachment #660873 - Flags: review?(rcampbell)
Attachment #662563 - Flags: review?(rcampbell)
Blocks: 792855
Priority: -- → P1
Comment on attachment 662563 [details] [diff] [review]
Patch v2

Good catch, Glandium.
Attachment #662563 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/f0ddac4f8973
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Depends on: 797336
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.