Last Comment Bug 837723 - [jsdbg2] Debugger should provide a way to get Debugger.Object referents directly
: [jsdbg2] Debugger should provide a way to get Debugger.Object referents directly
Status: RESOLVED FIXED
[firebug-p1]
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: x86 Windows Vista
: -- normal (vote)
: Firefox 23
Assigned To: Jim Blandy :jimb
:
Mentors:
Depends on:
Blocks: 783499 800200
  Show dependency treegraph
 
Reported: 2013-02-04 08:41 PST by Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08
Modified: 2013-05-14 16:51 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement Debugger.Object.prototype.unsafeDereference. (1.67 KB, patch)
2013-02-06 11:26 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Error message from CLANG (4.57 KB, text/plain)
2013-02-11 14:35 PST, Jim Blandy :jimb
no flags Details
Implement Debugger.Object.prototype.unsafeDereference. r=jorendorff (1.63 KB, patch)
2013-02-12 15:02 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Implement Debugger.Object.prototype.unsafeDereference. r=jorendorff (12.50 KB, patch)
2013-04-01 12:12 PDT, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review

Description Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08 2013-02-04 08:41:37 PST
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
Comment 1 Jim Blandy :jimb 2013-02-06 11:24:41 PST
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.
Comment 2 Jim Blandy :jimb 2013-02-06 11:26:02 PST
Created attachment 710850 [details] [diff] [review]
Implement Debugger.Object.prototype.unsafeDereference.
Comment 3 Jason Orendorff [:jorendorff] 2013-02-07 06:46:11 PST
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(...); ?
Comment 4 Jason Orendorff [:jorendorff] 2013-02-07 06:46:53 PST
(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 Victor Porof [:vporof][:vp] 2013-02-11 03:24:14 PST
(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
Comment 6 Jim Blandy :jimb 2013-02-11 14:32:56 PST
(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.
Comment 7 Jim Blandy :jimb 2013-02-11 14:35:16 PST
Created attachment 712632 [details]
Error message from CLANG

(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 Mihai Sucan [:msucan] 2013-02-12 05:23:22 PST
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.
Comment 9 Jim Blandy :jimb 2013-02-12 15:02:51 PST
Created attachment 713136 [details] [diff] [review]
Implement Debugger.Object.prototype.unsafeDereference. r=jorendorff

Updated to fix error caught by CLANG.
Comment 10 Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08 2013-02-28 08:39:15 PST
The patch works great for Firebug. Could we land it?
Honza
Comment 11 Jim Blandy :jimb 2013-03-07 15:07:38 PST
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.
Comment 12 Jim Blandy :jimb 2013-03-26 21:57:37 PDT
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
Comment 13 Jim Blandy :jimb 2013-03-28 13:00:13 PDT
Bobby Holley tells me that this problem was fixed by bug 854558, landed recently.
Comment 14 Jim Blandy :jimb 2013-04-01 09:45:27 PDT
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.
Comment 15 Jim Blandy :jimb 2013-04-01 12:12:06 PDT
Created attachment 731972 [details] [diff] [review]
Implement Debugger.Object.prototype.unsafeDereference. r=jorendorff

This version adds an xpcshell test for inter-privilege-domain references, and a mochitest that specifically detects xray wrapper behavior.
Comment 16 Jim Blandy :jimb 2013-04-01 12:58:56 PDT
Mihai, Jan, once this has baked, should I nominate it for Aurora?
Comment 17 Mihai Sucan [:msucan] 2013-04-01 13:32:25 PDT
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.
Comment 18 Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08 2013-04-02 03:35:12 PDT
(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 Jason Orendorff [:jorendorff] 2013-04-05 15:28:28 PDT
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.
Comment 20 Jim Blandy :jimb 2013-04-08 09:57:32 PDT
(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...
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-04-09 11:36:45 PDT
https://hg.mozilla.org/mozilla-central/rev/07aae4e9a66a
Comment 23 Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08 2013-04-09 23:39:13 PDT
Awesome!

Honza
Comment 24 Jim Blandy :jimb 2013-04-22 09:23:22 PDT
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 Mihai Sucan [:msucan] 2013-04-22 09:32:00 PDT
We should probably CC Will for dev-doc-needed bugs (devtools-related ones).

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