Last Comment Bug 749316 - Put Debugger object into chrome scratchpad
: Put Debugger object into chrome scratchpad
Status: RESOLVED FIXED
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: Developer Tools: Scratchpad (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on:
Blocks: 750364
  Show dependency treegraph
 
Reported: 2012-04-26 12:37 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-05-31 12:39 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (6.53 KB, patch)
2012-04-26 12:42 PDT, Jason Orendorff [:jorendorff]
dcamp: review+
bobbyholley: review+
Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2012-04-26 12:37:21 PDT
Steps to reproduce:

1. Set pref devtools.chrome.enabled to true
2. Visit http://www.squarefree.com/shell/shell.html
3. Open scratchpad; select menu Environment -> Browser.
4. Paste this code into Scratchpad and run it (Ctrl+R or Cmd+R).

    var dbg = new Debugger(gBrowser.selectedBrowser.contentWindow);
    dbg.onDebuggerStatement = function (frame) { alert("hello world"); }

5. In the squarefree tab, type "debugger" and hit enter.

Expected: an alert that says "hello world"

Observed: step 4 fails with:
    /*
    Exception: Debugger is not defined
    @Scratchpad:1
    */
Comment 1 Jason Orendorff [:jorendorff] 2012-04-26 12:42:30 PDT
Created attachment 618771 [details] [diff] [review]
v1
Comment 2 Jason Orendorff [:jorendorff] 2012-04-26 12:43:52 PDT
Comment on attachment 618771 [details] [diff] [review]
v1

r?bholley for the C++/IDL.
r?dcamp for everything else.
Comment 3 Bobby Holley (busy) 2012-04-26 13:51:55 PDT
Comment on attachment 618771 [details] [diff] [review]
v1


>diff --git a/js/ductwork/debugger/IJSDebugger.idl b/js/ductwork/debugger/IJSDebugger.idl
>--- a/js/ductwork/debugger/IJSDebugger.idl
>+++ b/js/ductwork/debugger/IJSDebugger.idl

> [scriptable, uuid(2fc14cc6-4ed0-4bbf-a7dd-e535bf088eb5)]
> interface IJSDebugger : nsISupports
> {
>   /**
>-   * Define the global Debugger constructor.
>+   * Define the global Debugger constructor on a given global.
>    */
>   [implicit_jscontext]
>-  void addClass();
>+  void addClass(in jsval global);
> };

Needs an iid rev.

>diff --git a/js/ductwork/debugger/JSDebugger.cpp b/js/ductwork/debugger/JSDebugger.cpp

>+  JSObject* obj = &global.toObject();
>+  obj = JS_UnwrapObject(obj);

Just pass , /* stopAtOuter = */ false here, skip the subsequent innerization, and throw if the result doesn't have the global flag. 

>+  if (!obj) {
>+    return NS_ERROR_FAILURE;
>   }
> 
>-  if (!JS_DefineDebuggerObject(cx, global)) {
>-    return NS_ERROR_FAILURE;
>+  {
>+    JSAutoEnterCompartment aec;
>+    if (!aec.enter(cx, obj)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+
>+    obj = JS_ObjectToInnerObject(cx, obj);
>+    if (!obj) {
>+      return NS_ERROR_FAILURE;
>+    }
>+  }
>+
>+  {
>+    JSAutoEnterCompartment aec;

I'd prefer ac2. But the entire first AutoEnterCompartment is unnecessary given the above. So let's just unwrap, enter the compartment, check that it's the global, and define the object.

r=bholley with that.
Comment 4 Jason Orendorff [:jorendorff] 2012-05-01 08:16:56 PDT
(In reply to Bobby Holley (:bholley) from comment #3)
> Needs an iid rev.

Oops. Thanks.

> >+  JSObject* obj = &global.toObject();
> >+  obj = JS_UnwrapObject(obj);
> 
> Just pass , /* stopAtOuter = */ false here, skip the subsequent
> innerization, and throw if the result doesn't have the global flag. 

JS_UnwrapObject doesn't have an optional second param. Actually I think it's extern "C" and used from C code (barf). So I added another function, JS_UnwrapObjectAndInnerize.

Ready to land when the tree reopens.
Comment 5 Bobby Holley (busy) 2012-05-01 08:32:03 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> JS_UnwrapObject doesn't have an optional second param. Actually I think it's
> extern "C" and used from C code (barf). So I added another function,
> JS_UnwrapObjectAndInnerize.

Erm, any reason not to just kill the extern C stuff and use js::UnwrapObject from jsfriendapi? Or maybe move it to jsapi if need be? Seems silly to have both.
Comment 7 Rob Campbell [:rc] (:robcee) 2012-05-07 08:02:29 PDT
this was landed directly on central?

Bobby, do we need a follow-up to address your question in Comment 5 or can this stick as-landed?

This also looks like something that could use some documentation.
Comment 8 Bobby Holley (busy) 2012-05-07 08:42:21 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #7)
> this was landed directly on central?
> 
> Bobby, do we need a follow-up to address your question in Comment 5 or can
> this stick as-landed?

These shims are silly, but there's no duplicated code, so I don't really mind.

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