Closed Bug 741615 Opened 8 years ago Closed 8 years ago

Replace Debugger.prototype.wrap with something more compartment-sensitive

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jorendorff, Assigned: jimb)

Details

Attachments

(1 file, 1 obsolete file)

Debugger.prototype.wrap is supposed to be able to wrap a debuggee object, but due to a bug in the implementation it wraps the debugger's cross-compartment wrapper of the debuggee object instead.

This is easiest to explain using a test:



var g = newGlobal('new-compartment');
g.eval("function f() { debugger; }");
var dbg = Debugger(g);
var jsonw;
dbg.onDebuggerStatement = function (frame) {
    jsonw = frame.eval("JSON").return;
};
g.eval("debugger;");
assertEq(dbg.wrap(g.JSON), jsonw);  // FAILS
At first the consensus was that this was a bug and easy to fix.

Now the consensus is that the .wrap() API has two problems:

  1. When you call dbg.wrap(obj) and obj is a cross-compartment wrapper,
     it isn't clear if you want a Debugger.Object whose referent is obj,
     or whose referent is the object which obj wraps.

  2. dbg.wrap({}) makes a non-cross-compartment Debugger.Object, something
     which billm wants to ban.

and we can fix them both by replacing it with a slightly different API.

jimb will spec it, it'll be Debugger.Object.prototype.wrap (possibly with a different name as -- shockingly -- "wrap" has proven a little bit tricky in practice). It's just like this dbg.wrap, but it always produces a Debugger.Object whose referent is in the same compartment as "this" Debugger.Object.
Summary: Debugger.prototype.wrap wraps the wrapper → Replace Debugger.prototype.wrap with something more compartment-sensitive
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> jimb will spec it,

"... because we've not yet found a way to prevent him from doing that sort of thing..."
Docs for change in patch: https://github.com/jimblandy/DebuggerDocs/commit/04bea716b4d598f0dffbf5de5ba2aa6a97368917
Assignee: general → jimb
Status: NEW → ASSIGNED
Attachment #612450 - Flags: review?(jorendorff)
Flags: in-testsuite+
(Sorry, forgot to refresh the patch...)
Attachment #612450 - Attachment is obsolete: true
Attachment #612450 - Flags: review?(jorendorff)
Attachment #612452 - Flags: review?(jorendorff)
Part of the inspiration for this was that I had hoped to stop using weakmaps in the debugger, and thus to simplify the weakmap internals a lot. However, I actually wrote the weakmap patch tonight, and it doesn't help as much as I'd hoped.

I still want to add Debugger.Objects to the cross-compartment wrapper map. But now I'm thinking it probably makes sense to leave them in debugger weakmaps as well. In that case, there's no reason that Debugger.wrap can't refer to debugger objects.

Anyway, I'm sorry I pushed you guys so aggressively to work on this bug. I think the only reason to do it now is if it makes your work easier. (Although I guess there's still a bug here, in comment 0, but that should be easy to fix.)
Comment on attachment 612452 [details] [diff] [review]
Replace Debugger.prototype.wrap with Debugger.Object.prototype.makeDebuggeeValue.

>+assertEq(gw.makeDebuggeeValue(g.JSON), jsonw);  // FAILS

Remove the comment. :)

Even given comment 6, I think we probably want this change. Most of the work was deciding what we wanted. r=me.
Attachment #612452 - Flags: review?(jorendorff) → review+
I kind of think this is the better API, so I agree we still want the patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b28f9e01c51
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/0b28f9e01c51
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.