Last Comment Bug 741615 - Replace Debugger.prototype.wrap with something more compartment-sensitive
: Replace Debugger.prototype.wrap with something more compartment-sensitive
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-02 15:36 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-04-06 11:33 PDT (History)
4 users (show)
jimb: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replace Debugger.prototype.wrap with Debugger.Object.prototype.makeDebuggeeValue. (4.27 KB, patch)
2012-04-04 21:55 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Replace Debugger.prototype.wrap with Debugger.Object.prototype.makeDebuggeeValue. (4.98 KB, patch)
2012-04-04 21:57 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-04-02 15:36:17 PDT
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
Comment 1 Jason Orendorff [:jorendorff] 2012-04-03 18:49:30 PDT
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.
Comment 2 Jim Blandy :jimb 2012-04-03 18:53:38 PDT
(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..."
Comment 3 Jim Blandy :jimb 2012-04-04 21:55:16 PDT
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
Comment 4 Jim Blandy :jimb 2012-04-04 21:57:32 PDT
Created attachment 612452 [details] [diff] [review]
Replace Debugger.prototype.wrap with Debugger.Object.prototype.makeDebuggeeValue.

(Sorry, forgot to refresh the patch...)
Comment 5 Jim Blandy :jimb 2012-04-04 21:59:33 PDT
try: https://tbpl.mozilla.org/?tree=Try&rev=d79509ebfcbe
Comment 6 [PTO to Dec5] Bill McCloskey (:billm) 2012-04-04 22:35:07 PDT
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 7 Jason Orendorff [:jorendorff] 2012-04-05 09:50:15 PDT
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.
Comment 8 Jim Blandy :jimb 2012-04-05 12:28:33 PDT
I kind of think this is the better API, so I agree we still want the patch.
Comment 10 Matt Brubeck (:mbrubeck) 2012-04-06 11:33:32 PDT
https://hg.mozilla.org/mozilla-central/rev/0b28f9e01c51

Note You need to log in before you can comment on or make changes to this bug.