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

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jimb)

Tracking

Other Branch
mozilla14
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
(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..."
(Assignee)

Comment 3

5 years ago
Created attachment 612450 [details] [diff] [review]
Replace Debugger.prototype.wrap with Debugger.Object.prototype.makeDebuggeeValue.

Docs for change in patch: https://github.com/jimblandy/DebuggerDocs/commit/04bea716b4d598f0dffbf5de5ba2aa6a97368917
Assignee: general → jimb
Status: NEW → ASSIGNED
Attachment #612450 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
(Assignee)

Comment 4

5 years ago
Created attachment 612452 [details] [diff] [review]
Replace Debugger.prototype.wrap with Debugger.Object.prototype.makeDebuggeeValue.

(Sorry, forgot to refresh the patch...)
Attachment #612450 - Attachment is obsolete: true
Attachment #612450 - Flags: review?(jorendorff)
Attachment #612452 - Flags: review?(jorendorff)
(Assignee)

Comment 5

5 years ago
try: https://tbpl.mozilla.org/?tree=Try&rev=d79509ebfcbe
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.)
(Reporter)

Comment 7

5 years ago
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+
(Assignee)

Comment 8

5 years ago
I kind of think this is the better API, so I agree we still want the patch.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b28f9e01c51
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/0b28f9e01c51
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.