Closed
Bug 790952
Opened 12 years ago
Closed 12 years ago
Debugger server always creates new actor instances
Categories
(DevTools :: Debugger, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 18
People
(Reporter: glandium, Assigned: past)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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 {
Assignee | ||
Comment 2•12 years ago
|
||
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.
Reporter | ||
Updated•12 years ago
|
Attachment #660873 -
Flags: feedback?(mh+mozilla) → feedback+
Assignee | ||
Updated•12 years ago
|
Attachment #660873 -
Flags: review?(rcampbell)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Priority: -- → P1
Comment 4•12 years ago
|
||
Comment on attachment 662563 [details] [diff] [review]
Patch v2
Good catch, Glandium.
Attachment #662563 -
Flags: review?(rcampbell) → review+
Assignee | ||
Comment 5•12 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 6•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•