Closed Bug 837723 Opened 11 years ago Closed 11 years ago

[jsdbg2] Debugger should provide a way to get Debugger.Object referents directly

Categories

(DevTools :: Debugger, defect)

x86
Windows Vista
defect
Not set
normal

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)

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
Summary: Introduce a new method in IJSDebugge allowing to get DO referent. → Introduce a new method in IJSDebugger allowing to get DO referent.
Whiteboard: [firebug-p1]
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: nobody → jimb
Status: NEW → ASSIGNED
Attachment #710850 - Flags: review?(jorendorff)
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+
(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.
Blocks: 783499
(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
(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.
(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.
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.
Updated to fix error caught by CLANG.
Attachment #710850 - Attachment is obsolete: true
The patch works great for Firebug. Could we land it?
Honza
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.
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
Bobby Holley tells me that this problem was fixed by bug 854558, landed recently.
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.
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)
Mihai, Jan, once this has baked, should I nominate it for Aurora?
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.
(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 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+
(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...
https://hg.mozilla.org/mozilla-central/rev/07aae4e9a66a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Awesome!

Honza
Keywords: dev-doc-needed
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.
We should probably CC Will for dev-doc-needed bugs (devtools-related ones).
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.