Closed
Bug 741615
Opened 12 years ago
Closed 12 years ago
Replace Debugger.prototype.wrap with something more compartment-sensitive
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: jorendorff, Assigned: jimb)
Details
Attachments
(1 file, 1 obsolete file)
4.98 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 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•12 years ago
|
||
Docs for change in patch: https://github.com/jimblandy/DebuggerDocs/commit/04bea716b4d598f0dffbf5de5ba2aa6a97368917
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 4•12 years ago
|
||
(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•12 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•12 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•12 years ago
|
||
I kind of think this is the better API, so I agree we still want the patch.
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b28f9e01c51
Target Milestone: --- → mozilla14
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0b28f9e01c51
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•