Closed
Bug 837723
Opened 12 years ago
Closed 12 years ago
[jsdbg2] Debugger should provide a way to get Debugger.Object referents directly
Categories
(DevTools :: Debugger, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: Honza, Assigned: jimb)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [firebug-p1])
Attachments
(2 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
To help Firebug's JSD2 adoption (transition away from JSD), it would be great if it's possible to access JSD2 debugger-object's referent (the real JS object).
This way Firebug could adopt JSD2 panel-by-panel while keeping integration among panels working.
This API could be also useful in cases where integration between remotable and non-remotable tools is required.
Honza
Reporter | ||
Updated•12 years ago
|
Summary: Introduce a new method in IJSDebugge allowing to get DO referent. → Introduce a new method in IJSDebugger allowing to get DO referent.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [firebug-p1]
Assignee | ||
Comment 1•12 years ago
|
||
After some discussion, it seems to me that we might as well just make this a method of Debugger.Object, directly.
While there are good reasons for wanting Debugger.Object to usually act as a protective membrane between the debugger and possibly hostile debuggee code, and while we should make that membrane as useful as we can see how, that protection should be a service, not a restriction. If code using Debugger is willing to accept the consequences, and if membrane-piercing code is easily identified, then it doesn't seem to be a terrible thing.
Bug title changed appropriately.
Summary: Introduce a new method in IJSDebugger allowing to get DO referent. → [jsdbg2] Debugger should provide a way to get Debugger.Object referents directly
Assignee | ||
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
Comment on attachment 710850 [details] [diff] [review]
Implement Debugger.Object.prototype.unsafeDereference.
Review of attachment 710850 [details] [diff] [review]:
-----------------------------------------------------------------
This could use a mochitest, to make sure that when a chrome compartment dereferences a D.O whose referent is a content DOM node, the result is an x-ray wrapper (that is, it does not see content expando properties on the real referent).
(That could be tested in the shell but it wouldn't be easy, I think.)
::: js/src/vm/Debugger.cpp
@@ +4618,5 @@
> + THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "unsafeDereference", args, referent);
> + args.rval().setObject(*referent);
> + if (!cx->compartment->wrap(cx, args.rval().address()))
> + return false;
> + return true;
just return cx->compartment->wrap(...); ?
Attachment #710850 -
Flags: review?(jorendorff) → review+
Comment 4•12 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> (That could be tested in the shell but it wouldn't be easy, I think.)
I mean, it could be tested in the shell with enough C++ scaffolding... which wouldn't be very easy to build.
Comment 5•12 years ago
|
||
(In reply to Jim Blandy :jimb from comment #2)
> Created attachment 710850 [details] [diff] [review]
> Implement Debugger.Object.prototype.unsafeDereference.
> + if (!cx->compartment->wrap(cx, args.rval().address()))
This makes clang version 3.2 die with the following error: no matching member function for call to 'wrap' [0]
[0] http://www.pastebin.mozilla.org/2131574
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Victor Porof [:vp] from comment #5)
> (In reply to Jim Blandy :jimb from comment #2)
> > Created attachment 710850 [details] [diff] [review]
> > Implement Debugger.Object.prototype.unsafeDereference.
>
> > + if (!cx->compartment->wrap(cx, args.rval().address()))
>
> This makes clang version 3.2 die with the following error: no matching
> member function for call to 'wrap' [0]
>
> [0] http://www.pastebin.mozilla.org/2131574
Ah, thanks. I think I can correct that.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Victor Porof [:vp] from comment #5)
> [0] http://www.pastebin.mozilla.org/2131574
Let's actually attach stuff like this directly to bugs, instead of citing external sites that will go away.
Comment 8•12 years ago
|
||
This patch worked for me until today, when I pulled from fx-team. I have clang 3.1 and I get the same error as Victor, on Linux.
Assignee | ||
Comment 9•12 years ago
|
||
Updated to fix error caught by CLANG.
Attachment #710850 -
Attachment is obsolete: true
Reporter | ||
Comment 10•12 years ago
|
||
The patch works great for Firebug. Could we land it?
Honza
Assignee | ||
Comment 11•12 years ago
|
||
I need to figure out how to add that test jorendorff requested; I started work on it, and then set it aside. I'll try to get to it soon.
Assignee | ||
Comment 12•12 years ago
|
||
Below is a message sent to Bobby Holley:
---
I've been working on a test case for bug 837723, and have run into some behavior that's surprising to me.
In the test script below, I create a sandbox with content privileges, and one with chrome. Or at least, that was my intention; evidently I've done something different, but I don't know what.
I would expect the content-privileged sandbox, contentBox, to be able to see neither properties of objects in the chrome-privileged sandbox, chromeBox, nor properties of objects in the main compartment. However, it seems that contentBox cannot see properties of objects in the main compartment, but *can* see properties of objects in chromeBox.
In other words, all tests here pass, until the very last one.
What have I done, here? What privileges do the main, contentBox, and chromeBox compartments have?
Will any of them create xray wrappers for the others, in xpcshell? That's what I'm really aiming to test, but I ran into this confusion before I could get to that point.
// Any copyright is dedicated to the Public Domain.
// http://creativecommons.org/publicdomain/zero/1.0/
// Test Debugger.Object.prototype.unsafeDereference in the presence of
// interesting cross-compartment wrappers.
//
// This is not really a debugger server test; it's more of a Debugger test.
// But we need xpcshell and Components.utils.Sandbox to get
// cross-compartment wrappers with interesting properties, and this is the
// xpcshell test directory most closely related to the JS Debugger API.
function run_test() {
// Create a low-privilege sandbox, and a chrome-privilege sandbox.
let contentBox = Components.utils.Sandbox('http://www.example.com');
let chromeBox = Components.utils.Sandbox(this);
// Create an objects in this compartment, and one in each sandbox. We'll
// refer to the objects as "mainObj", "contentObj", and "chromeObj", in
// variable and property names.
var mainObj = { name: "mainObj" };
Components.utils.evalInSandbox('var contentObj = { name: "contentObj" };',
contentBox);
Components.utils.evalInSandbox('var chromeObj = { name: "chromeObj" };',
chromeBox);
// Give each global a pointer to all the other globals' objects.
contentBox.mainObj = chromeBox.mainObj = mainObj;
var contentObj = chromeBox.contentObj = contentBox.contentObj;
var chromeObj = contentBox.chromeObj = chromeBox.chromeObj;
dump('contentBox.contentObj: ' + uneval(contentBox.contentObj) + '\n');
dump('contentBox.mainObj: ' + uneval(contentBox.mainObj) + '\n');
dump('contentBox.chromeObj: ' + uneval(contentBox.chromeObj) + '\n');
// The objects appear as global variables in the sandbox, and as
// the sandbox object's properties in chrome.
do_check_true(Components.utils.evalInSandbox('mainObj', contentBox)
=== contentBox.mainObj);
do_check_true(Components.utils.evalInSandbox('contentObj', contentBox)
=== contentBox.contentObj);
do_check_true(Components.utils.evalInSandbox('chromeObj', contentBox)
=== contentBox.chromeObj);
// We (the main global) can see properties of all objects in all globals.
do_check_true(contentBox.mainObj.name === "mainObj");
do_check_true(contentBox.contentObj.name === "contentObj");
do_check_true(contentBox.chromeObj.name === "chromeObj");
// chromeBox can see properties of all objects in all globals.
do_check_eq(Components.utils.evalInSandbox('mainObj.name', chromeBox),
'mainObj');
do_check_eq(Components.utils.evalInSandbox('contentObj.name', chromeBox),
'contentObj');
do_check_eq(Components.utils.evalInSandbox('chromeObj.name', chromeBox),
'chromeObj');
// contentBox can see properties of the content object, but not of either
// chrome object, because by default, content -> chrome wrappers hide all
// object properties.
do_check_eq(Components.utils.evalInSandbox('mainObj.name', contentBox),
undefined);
do_check_eq(Components.utils.evalInSandbox('contentObj.name', contentBox),
'contentObj');
do_check_eq(Components.utils.evalInSandbox('chromeObj.name', contentBox),
undefined);
}
Here's the output I get when I run this:
-*- mode: compilation; default-directory: "~/moz/mc/" -*-
Compilation started at Tue Mar 26 17:57:43
cd ~/moz/mc && moz xpcshell -- -f $dbg/testing/xpcshell/head.js -e '_HEAD_FILES=["'$dbg'/toolkit/devtools/debugger/tests/unit/head_dbg.js"]' -e '_TEST_FILE = ["'$dbg'/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js"];' -e 'var _TAIL_FILES = [];' -e '_execute_test(); quit(0);'
obj='/home/jimb/moz/mc/obj-rel'
# I don't know what -m -n -s do. It's just what the test harness passes.
LD_LIBRARY_PATH=$obj/dist/bin:/usr/local/lib \
exec $wrapper $obj/dist/bin/xpcshell -g $obj/dist/bin -a $obj/dist/bin -m -n -s "$@"
TEST-INFO | (xpcshell/head.js) | test 1 pending
contentBox.contentObj: ({name:"contentObj"})
contentBox.mainObj: ({name:"mainObj"})
contentBox.chromeObj: ({name:"chromeObj"})
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 38] true == true
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 40] true == true
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 42] true == true
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 45] true == true
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 46] true == true
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 47] true == true
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 51] mainObj == mainObj
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 53] contentObj == contentObj
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 55] chromeObj == chromeObj
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 61] undefined == undefined
TEST-PASS | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | [run_test : 63] contentObj == contentObj
TEST-UNEXPECTED-FAIL | /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js | chromeObj == undefined - See following stack:
JS frame :: /home/jimb/moz/dbg/testing/xpcshell/head.js :: do_throw :: line 461
JS frame :: /home/jimb/moz/dbg/testing/xpcshell/head.js :: do_report_result :: line 563
JS frame :: /home/jimb/moz/dbg/testing/xpcshell/head.js :: _do_check_eq :: line 573
JS frame :: /home/jimb/moz/dbg/testing/xpcshell/head.js :: do_check_eq :: line 580
JS frame :: /home/jimb/moz/dbg/toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js :: run_test :: line 65
JS frame :: /home/jimb/moz/dbg/testing/xpcshell/head.js :: _execute_test :: line 325
JS frame :: -e :: <TOP_LEVEL> :: line 1
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
TEST-INFO | (xpcshell/head.js) | exiting test
Compilation finished at Tue Mar 26 17:57:43
Assignee | ||
Comment 13•12 years ago
|
||
Bobby Holley tells me that this problem was fixed by bug 854558, landed recently.
Assignee | ||
Comment 14•12 years ago
|
||
I've gotten some mochitest help from Boris Zbarski, so I believe I finally have what I need to write the xray wrapper test requested in comment 3.
Assignee | ||
Comment 15•12 years ago
|
||
This version adds an xpcshell test for inter-privilege-domain references, and a mochitest that specifically detects xray wrapper behavior.
Attachment #713136 -
Attachment is obsolete: true
Attachment #731972 -
Flags: review?(jorendorff)
Assignee | ||
Comment 16•12 years ago
|
||
Mihai, Jan, once this has baked, should I nominate it for Aurora?
Comment 17•12 years ago
|
||
Thanks for the updated patch!
(In reply to Jim Blandy :jimb from comment #16)
> Mihai, Jan, once this has baked, should I nominate it for Aurora?
For the web console stuff you do not need to nominate this patch for aurora. I need to land the console patches after this one lands.
Reporter | ||
Comment 18•12 years ago
|
||
(In reply to Jim Blandy :jimb from comment #16)
> Mihai, Jan, once this has baked, should I nominate it for Aurora?
Yes please, it would be useful for Firebug.
Thanks!
Honza
Comment 19•12 years ago
|
||
Comment on attachment 731972 [details] [diff] [review]
Implement Debugger.Object.prototype.unsafeDereference. r=jorendorff
Review of attachment 731972 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/devtools/debugger/tests/mochitest/test_unsafeDereference.html
@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=837723
> +
> +When we use Debugger.Object.prototype.unsafeDereference to get a non-D.O
> +reference to a content object in chrome, that reference should be via an
trailing whitespace
::: toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js
@@ +6,5 @@
> +//
> +// This is not really a debugger server test; it's more of a Debugger test.
> +// But we need xpcshell and Components.utils.Sandbox to get
> +// cross-compartment wrappers with interesting properties, and this is the
> +// xpcshell test directory most closely related to the JS Debugger API.
There's an awful lot in this test that's just testing basic cross-compartment wrapper properties; is that stuff not adequately tested elsewhere? I'm OK with keeping it I guess.
@@ +103,5 @@
> + // same property visibility we checked for above.
> + let mainFromContentDO = contentBoxDO.getProperty('mainObj');
> + do_check_eq(mainFromContentDO, contentBoxDO.makeDebuggeeValue(mainObj));
> + do_check_eq(mainFromContentDO.getProperty('name'), undefined);
> + do_check_eq(mainFromContentDO.unsafeDereference(), mainObj);
I don't mind the third line of each of these blocks, as I know this stuff *isn't* well-tested. Good to have tests for it finally.
@@ +108,5 @@
> +
> + let contentFromContentDO = contentBoxDO.getProperty('contentObj');
> + do_check_eq(contentFromContentDO, contentBoxDO.makeDebuggeeValue(contentObj));
> + do_check_eq(contentFromContentDO.getProperty('name'), 'contentObj');
> + do_check_eq(contentFromContentDO.unsafeDereference(), contentObj);
The blocks where we look at contentObj from content and chromeObj from chrome seem a bit silly though.
Attachment #731972 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #19)
> Comment on attachment 731972 [details] [diff] [review]
> Implement Debugger.Object.prototype.unsafeDereference. r=jorendorff
>
> Review of attachment 731972 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: toolkit/devtools/debugger/tests/mochitest/test_unsafeDereference.html
> @@ +3,5 @@
> > +<!--
> > +https://bugzilla.mozilla.org/show_bug.cgi?id=837723
> > +
> > +When we use Debugger.Object.prototype.unsafeDereference to get a non-D.O
> > +reference to a content object in chrome, that reference should be via an
>
> trailing whitespace
Fixed, thanks.
> ::: toolkit/devtools/debugger/tests/unit/test_unsafeDereference.js
> @@ +6,5 @@
> > +//
> > +// This is not really a debugger server test; it's more of a Debugger test.
> > +// But we need xpcshell and Components.utils.Sandbox to get
> > +// cross-compartment wrappers with interesting properties, and this is the
> > +// xpcshell test directory most closely related to the JS Debugger API.
>
> There's an awful lot in this test that's just testing basic
> cross-compartment wrapper properties; is that stuff not adequately tested
> elsewhere? I'm OK with keeping it I guess.
Right, that was me verifying that things behaved the way I expected them to, before I introduce Debugger into the mix. In fact, sandboxes had strange behavior which had only just been fixed (see comment 13), which I only noticed because I checked the basic behavior first.
So, I'm sure it is adequately tested elsewhere, but it was useful orientation for me. Perhaps it will be valuable to future readers of the test who are not entirely sure how wrappers will behave in xpcshell.
> @@ +103,5 @@
> > + // same property visibility we checked for above.
> > + let mainFromContentDO = contentBoxDO.getProperty('mainObj');
> > + do_check_eq(mainFromContentDO, contentBoxDO.makeDebuggeeValue(mainObj));
> > + do_check_eq(mainFromContentDO.getProperty('name'), undefined);
> > + do_check_eq(mainFromContentDO.unsafeDereference(), mainObj);
>
> I don't mind the third line of each of these blocks, as I know this stuff
> *isn't* well-tested. Good to have tests for it finally.
>
> @@ +108,5 @@
> > +
> > + let contentFromContentDO = contentBoxDO.getProperty('contentObj');
> > + do_check_eq(contentFromContentDO, contentBoxDO.makeDebuggeeValue(contentObj));
> > + do_check_eq(contentFromContentDO.getProperty('name'), 'contentObj');
> > + do_check_eq(contentFromContentDO.unsafeDereference(), contentObj);
>
> The blocks where we look at contentObj from content and chromeObj from
> chrome seem a bit silly though.
It's not nice to be sure we're not introducing wrappers where there should be none? We only presume that the DO has the point of view we want...
Assignee | ||
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Reporter | ||
Comment 23•12 years ago
|
||
Awesome!
Honza
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 24•12 years ago
|
||
David, for what it's worth, I've added documentation for unsafeDereference to https://wiki.mozilla.org/Debugger. The dev-doc-needed keyword probably indicates a need for an update on https://developer.mozilla.org/en-US/docs/SpiderMonkey/JS_Debugger_API_Reference/Debugger, so I'll leave that set.
Comment 25•12 years ago
|
||
We should probably CC Will for dev-doc-needed bugs (devtools-related ones).
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•