Closed Bug 1282257 Opened 8 years ago Closed 8 years ago

Expose the target and the handler of a proxy

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(1 file, 10 obsolete files)

13.02 KB, patch
jimb
: review+
Details | Diff | Splinter Review
For bug 1274657 we need some way to get the target and the handler of a proxy.

I think currently there is no way, so inspired by the existing Components.utils.isProxy, I propose adding a pair of new methods.

My knowledge of C++ and Firefox internals is low, so I would appreciate some feedback. I don't know who am I supposed to ask for, so I choose jorendorff because bug 1274657 was his idea.
Attachment #8765204 - Flags: feedback?(jorendorff)
Comment on attachment 8765204 [details] [diff] [review]
Create methods in Components.utils to expose the target and the handler of a proxy object

Review of attachment 8765204 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, Oriol. I think we should add these methods to the Debugger.Object object instead (see js/src/vm/Debugger.cpp and js/src/jit-test/tests/debug). I'd like a second opinion, though, so I'm forwarding this feedback request to Jim.
Attachment #8765204 - Flags: feedback?(jorendorff) → feedback?(jimb)
Agreed that this should be on Debugger.Object.
Whoa, looking in js/src/doc/Debugger/Debugger.Object.md, I see specification text for accessor properties Debugger.Object.prototype.{proxyHandler,proxyCallTrap,proxyConstructTrap}, none of which are actually implemented. That's obsolete. What we actually would want is .proxyTarget and .proxyHandler.
I'm on vacation, so I won't be providing feedback on this until Tuesday, July 5. However, Jason and Nick are excellent, um, proxies for me until then. In particular, comment 3 seems on the mark.
Comment on attachment 8765204 [details] [diff] [review]
Create methods in Components.utils to expose the target and the handler of a proxy object

Thanks for the patch! However, this should be revised per comment 3.
Attachment #8765204 - Flags: feedback?(jimb) → feedback-
Oriol, since you figured out the coding conventions to add these methods to Components.utils, you may find it easy enough adding a few getters to Debugger.Object (and testing them).

If you have any trouble at all, leave a message here. (You can also join us on IRC: <https://wiki.mozilla.org/IRC>. JavaScript engine hackers hang out in #jsapi on Mozilla IRC.)
I think it should be something like this.

I wasn't sure if the accessors should return the raw objects or debugger object wrappers, I guessed the later.

I noticed a weird thing. If I open a privileged console and enter

    Components.utils.import('resource://gre/modules/jsdebugger.jsm');
    addDebuggerToGlobal(this);
    var iframe = document.createElement("iframe");
    document.body.appendChild(iframe);

and then enter

    var dbg = new Debugger().addDebuggee(iframe.contentWindow);
    dbg.makeDebuggeeValue([1,2,3]).isProxy;

I can't access the referent for some reason. In the patch I check

    if(!referent) return false;

to prevent a crash, but not sure if I should set the result to undefined or something.

If the two blocks of code are entered together, it works.
Attachment #8765204 - Attachment is obsolete: true
My repo was a bit old, this patch should merge properly with latest version.
Attachment #8771690 - Attachment is obsolete: true
(In reply to Oriol from comment #7)
> Created attachment 8771690 [details] [diff] [review]
> Create accessor properties in Debugger.Object.prototype to expose the target
> and the handler of a proxy object
> 
> I think it should be something like this.
> 
> I wasn't sure if the accessors should return the raw objects or debugger
> object wrappers, I guessed the later.
> 
> I noticed a weird thing. If I open a privileged console and enter
> 
>     Components.utils.import('resource://gre/modules/jsdebugger.jsm');
>     addDebuggerToGlobal(this);
>     var iframe = document.createElement("iframe");
>     document.body.appendChild(iframe);
> 
> and then enter
> 
>     var dbg = new Debugger().addDebuggee(iframe.contentWindow);
>     dbg.makeDebuggeeValue([1,2,3]).isProxy;
> 

This call to makeDebuggeeValue doesn't do what you expect, I think. You're expecting it to make an array in the debuggee's compartment, right? What it actually does is:
1) create an array in the privileged console's compartment,
2) pass it to the debuggee, producing a cross-compartment wrapper in the debuggee compartment referring to the array, which remains in the privileged compartment,
3) Create a Debugger.Object referring to that cross-compartment wrapper.

In general, unprivileged compartments can't see privileged compartments' objects, so you shouldn't be able to access the referent.

What you want to do is insert a script element into the iframe's document whose source creates an array, and stores it in a global variable. That will be visible to the debugger like this:

var dbg = new Debugger();
var windowDO = dbg.addDebuggee(iframe.contentWindow);
var arrayDO = windowDO.getOwnPropertyDescriptor('a').value;

However, for testing changes to Debugger, you should write unit tests in js/src/jit-test/tests/debug. The Object*.js tests in that directory should provide plenty of examples.
Thanks for the explanation, makes sense.

But I can access the referent when I run the two codes from comment 7 together. Is it supposed to work like that? It looks like the security restrictions are bypassed.

And should I do something special when I can't see the referent? It seems each method does something different. Some blatantly lie, some return null or undefined, and some throw.

    dbg.makeDebuggeeValue([]).errorMessageName; // Error: Permission denied to access object
    dbg.makeDebuggeeValue(Object.preventExtensions([])).isExtensible(); // true
    dbg.makeDebuggeeValue(Object.freeze([])).isFrozen(); // false
    dbg.makeDebuggeeValue(Object.seal([])).isSealed(); // false
    dbg.makeDebuggeeValue([]).proto; // null
    dbg.makeDebuggeeValue((function foo(){})).name; // undefined
I don't know why it behaves that way when you run the two together. That's a surprise to me. What's the value of this expression?

    dbg.makeDebuggeeValue([1,2,3]).class

I think what you're doing there is inspecting the wrapper itself. It's up to the embedding's security policy how all these accessors behave on wrappers.

You shouldn't do anything special in order to see the referent: the purpose of Debugger.Object is to present the object as content would see it. We can call the Debugger.Object's `unwrap` method if we want to traverse the wrapper.
See DebuggerObject::getClassName for an example of the kind of "gentle touch" we want Debugger.Object to apply to its referent. We don't unwrap anything, we don't look inside anything, we just apply GetObjectClassName directly to the referent pointer.

In that particular case, GetObjectClassName needs to have the context's current compartment be the referent's, but I don't *think* you should need that.

People sort of think of debuggers as aggressive things that rip off the covers and show what's inside; but they should only do that when explicitly requested. A Debugger.Object is meant to represent a JavaScript object *as content sees it*, so the D.O accessors should try to return the same answer that content could would if it were manipulating the object itself.
(In reply to Jim Blandy :jimb from comment #11)
> What's the value of this expression?
> 
>     dbg.makeDebuggeeValue([1,2,3]).class

If I run the codes together, I get "Array". Separately, I get "Object".

> We can call the Debugger.Object's `unwrap` method if we want to traverse the wrapper.

`unwrap` returns null when running the codes separately.

(In reply to Jim Blandy :jimb from comment #12)
> See DebuggerObject::getClassName for an example of the kind of "gentle
> touch" we want Debugger.Object to apply to its referent. We don't unwrap
> anything, we don't look inside anything, we just apply GetObjectClassName
> directly to the referent pointer.

OK, but with the proxy methods I can't call js::IsScriptedProxy without looking if I can access the referent. Otherwise Firefox crashes.
(In reply to Oriol from comment #13)
> (In reply to Jim Blandy :jimb from comment #11)
> > What's the value of this expression?
> > 
> >     dbg.makeDebuggeeValue([1,2,3]).class
> 
> If I run the codes together, I get "Array". Separately, I get "Object".
> 
> > We can call the Debugger.Object's `unwrap` method if we want to traverse the wrapper.
> 
> `unwrap` returns null when running the codes separately.

This definitely does not sound right. Could you file a separate bug for that?

> OK, but with the proxy methods I can't call js::IsScriptedProxy without
> looking if I can access the referent. Otherwise Firefox crashes.

Let me take a look at the patch; I don't think IsScriptedProxy should be crashing when used like that.

By the way: when you've got a patch you'd like feedback on, you should go to the patch, choose "Details", and select "feedback?" type ":jimb" or ":fitzgen" or ":shu" or ":jorendorff" in the field that pops up, let Bugzilla find our email, select that, and then submit the changes.

Bugzilla will then bug us until we look at the patch, and change the flag to feedback+ or feedback- and write comments for you.
Comment on attachment 8771782 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object

Review of attachment 8771782 [details] [diff] [review]:
-----------------------------------------------------------------

First, thanks for updating the patch for Eddy's changes. We're separating out a C++ API from the JavaScript API in preparation for making Rust bindings for Debugger, hence the isProxyGetter / isProxy separation.

This is really not bad at all. You seem to know what you're doing.

::: js/src/vm/Debugger.cpp
@@ +9686,5 @@
> +DebuggerObject::isProxy(JSContext* cx, Handle<DebuggerObject*> object, bool& result)
> +{
> +    RootedObject referent(cx, object->referent());
> +    referent = js::CheckedUnwrap(referent, /* stopAtWindowProxy = */ false);
> +    if(!referent) return false;

These are the lines that I don't think we should need. You said that IsScriptedProxy crashes without them? What's the backtrace?

@@ +9704,5 @@
> +        RootedObject unwrapped(cx, js::GetProxyTargetObject(referent));
> +        return dbg->wrapDebuggeeObject(cx, unwrapped, result);
> +    } else {
> +        result.set(nullptr);
> +        return true;

It's SpiderMonkey style to place early returns like this in the 'then' clause, and leave the continuation for the main spine of the function. The intent is simply to reduce variation in the way control flow gets expressed. So the way you handled the CheckedUnwrap call is correct (although that needs to come out, I'm pretty sure), and the rest would look like:

    if(!js::IsScriptedProxy(referent)) {
        result.set(nullptr);
        return true;
    }

    Debugger* dbg = object->owner();
    RootedObject unwrapped(cx, js::GetProxyTargetObject(referent));
    return dbg->wrapDebuggeeObject(cx, unwrapped, result);
Attachment #8771782 - Flags: feedback+
Comment on attachment 8771782 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object

Review of attachment 8771782 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +8309,5 @@
> +    Rooted<DebuggerObject*> result(cx);
> +    if (!DebuggerObject::proxyTarget(cx, object, &result))
> +        return false;
> +
> +    args.rval().setObjectOrNull(result);

It would be more consistent with the rest of the Debugger API for this to return undefined when result is nullptr.
OK, it crashed when I didn't check the referent because I was using CheckedUnwrap. I didn't know what it did, I just copied it from the existing Cu.isProxy.

I have removed that. Now, if it doesn't have access to the referent, it says it is not a proxy.

I have also removed the &result parameter of isProxy because now that it will always succeed, I think it can return the result instead of returning the successfulness.
Attachment #8771782 - Attachment is obsolete: true
Attachment #8772165 - Flags: review?(jimb)
(In reply to Oriol from comment #17)
> I have removed that. Now, if it doesn't have access to the referent, it says
> it is not a proxy.

Yeah, I think that's the right thing to do.
Comment on attachment 8772165 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object

Review of attachment 8772165 [details] [diff] [review]:
-----------------------------------------------------------------

This code looks great; I've made some comments below.

I'm rejecting the patch because it needs unit tests (see js/src/jit-test/tests/debug/Object-* for examples), and documentation fixes in js/src/doc/Debugger.

Please flag me when you've got those done, and I'll try to review promptly. Thanks very much!

::: js/src/vm/Debugger.cpp
@@ +9699,5 @@
> +    RootedObject referent(cx, object->referent());
> +    if(!js::IsScriptedProxy(referent)) {
> +        result.set(nullptr);
> +        return true;
> +    }

This should handle errors a bit differently.

As you've noticed, DebuggerObject has a JavaScript API (isProxyGetter, proxyTargetGetter, proxyHandlerGetter), and a C++ API (isProxy, proxyTarget, proxyHandler). We've got a similar split for the other sorts of accessors and methods on Debugger.Object, and we're introducing them incrementally for other Debugger.Foo types.

At the C++ level, our pattern is that we provide a predicate for specific types of referent (isProxy, isFunction, etc.), and then the accessors specific to that type simply MOZ_ASSERT that the referent is actually the right kind of thing. The idea is to require C++ code to first check the type, and then proceed accordingly. This is a common pattern in C++, so it should be familiar to C++ developers.

At the JavaScript level, our pattern is to provide an accessor property for specific types (isCallable, isProxy), and then provide accessors that return undefined when applied to a D.O whose referent isn't the right type.

So your DebuggerObject::isProxy is fine. But proxyTarget and proxyHandler should simply MOZ_ASSERT that the referent is a proxy (by calling isProxy); and the JavaScript accessor property getters like proxyTargetGetter should check that the referent is indeed a proxy (also by calling DebuggerObject::isProxy), return undefined if it isn't, and only otherwise call the C++ accessor (say, proxyTarget).
Attachment #8772165 - Flags: review?(jimb) → review-
(In reply to Jim Blandy :jimb from comment #19)
> But proxyTarget and proxyHandler
> should simply MOZ_ASSERT that the referent is a proxy (by calling isProxy);
> and the JavaScript accessor property getters like proxyTargetGetter should
> check that the referent is indeed a proxy (also by calling
> DebuggerObject::isProxy), return undefined if it isn't, and only otherwise
> call the C++ accessor (say, proxyTarget).

OK, I moved DebuggerObject::isProxy next to the other "Infallible properties" which also are used like this.

I added a test, but couldn't try it. "./mach test check-spidermonkey" complains "%1 is not a valid Win32 application". I think that's because it attempts to run "obj-i686-pc-mingw32\dist\bin\js.exe", which does not exist.

I tried to run the test manually with "jit_test.py", but it always says that the test passed, even if I use assertEq(1,2) or throw.
Attachment #8772165 - Attachment is obsolete: true
Attachment #8772475 - Flags: review?(jimb)
Comment on attachment 8772475 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object

Review of attachment 8772475 [details] [diff] [review]:
-----------------------------------------------------------------

I had some minor requests, but overall this looks great, so I'm giving it r+, but please don't land it until we sort out the difficulties you're having with the test suite.

::: js/src/doc/Debugger/Debugger.Object.md
@@ +190,5 @@
>  
> +`isProxy`
> +:   If the referent is a (scripted) proxy, return `true`. If the referent is not
> +    a proxy, return `false`. If the referent is a wrapper, it won't be
> +    unwrapped, so this is `false`.

I don't think you need to mention the wrapper case, because that applies to almost all the accessors on Debugger.Object.

Promises were implemented differently because the module system used in Firefox chrome code causes almost every promise to be used via a cross-compartment wrapper --- each module is a separate compartment; inter-compartment edges always go through wrappers; and chrome code uses a module to create its promises.

That doesn't apply to proxies, which have always been provided directly by SpiderMonkey.

@@ +196,5 @@
> +`proxyTarget`
> +:   If the referent is a (scripted) proxy, return a `Debugger.Object` instance
> +    referring to the ECMAScript `[[ProxyTarget]]` of the referent. If the referent
> +    is not a proxy, return `undefined`. If the referent is a wrapper, it won't be
> +    unwrapped, so this is `undefined`.

Similarly.

@@ +202,5 @@
>  `proxyHandler`
> +:   If the referent is a (scripted) proxy, return a `Debugger.Object` instance
> +    referring to the ECMAScript `[[ProxyHandler]]` of the referent. If the referent
> +    is not a proxy, return `undefined`. If the referent is a wrapper, it won't be
> +    unwrapped, so this is `undefined`.

Similarly.

::: js/src/jit-test/tests/debug/Object-proxy.js
@@ +10,5 @@
> +g.eval('var handler = {has: ()=>false};');
> +g.eval('var proxy = new Proxy(target, handler);');
> +g.eval('var proxyProxy = new Proxy(proxy, proxy);');
> +
> +var target = gDO.executeInGlobal('target').return;

Nit: it's a little less eval-intensive to write:

gDO.getOwnPropertyDescriptor('target').value

@@ +15,5 @@
> +var handler = gDO.executeInGlobal('handler').return;
> +var proxy = gDO.executeInGlobal('proxy').return;
> +var proxyProxy = gDO.executeInGlobal('proxyProxy').return;
> +
> +assertEq(false, target.isProxy);

The error messages from `assertEq` will say that the first argument is the "actual" value and the second is the "expected" value, so you should reverse the order of the arguments in your calls.

@@ +21,5 @@
> +assertEq(true, proxy.isProxy);
> +assertEq(true, proxyProxy.isProxy);
> +
> +assertEq(void 0, target.proxyTarget);
> +assertEq(void 0, handler.proxyTarget);

It's okay to use `undefined` here; it's a read-only, non-configurable property of the global, so it's trustworthy.
Attachment #8772475 - Flags: review?(jimb) → review+
Your test code looks totally fine to me. Can you tell me exactly how you're invoking js/src/jit-test/jit_test.py?
(In reply to Jim Blandy :jimb from comment #22)
> Your test code looks totally fine to me. Can you tell me exactly how you're
> invoking js/src/jit-test/jit_test.py?

I first tried

    ./js/src/jit-test/jit_test.py`

but it complains it needs a shell.

I don't know what kind of shell it wants, but I tried

    ./js/src/jit-test/jit_test.py ./obj-i686-pc-mingw32/dist/bin/jsapi-tests.exe > jstest.txt

It does some tests. After some time I didn't see any running jsapi-tests.exe process, but the console thought that the test were still running. So I cancelled. The log was flushed, but my test wasn't there.

Then I tried to run my test manually like this

    ./js/src/jit-test/jit_test.py ./obj-i686-pc-mingw32/dist/bin/jsapi-tests.exe Object-proxy.js

It detects my test and runs it and says PASSED ALL, even if I use assertEq(1,2).
Yes, when jit_test.py complains it needs a shell, it means that its first argument should be the path of the JavaScript shell you just built.

You can try running your test *very* manually by just writing:

./obj-i686-pc-mingw32/dist/bin/jsapi-tests.exe js/src/jit-test/tests/debug/Object-proxy.js

Technically, the harness sets a bunch of variables, but I don't think your test needs them.
BTW, you can find me on Mozilla IRC as 'jimb' in #jsapi.
(In reply to Oriol from comment #23)
> I don't know what kind of shell it wants, but I tried
> 
>     ./js/src/jit-test/jit_test.py
> ./obj-i686-pc-mingw32/dist/bin/jsapi-tests.exe > jstest.txt

(As explained on IRC, just putting it here for the record)

I didn't read carefully; jsapi-tests.exe is not the right executable. That's a separate test program, built to exercise the C++ API. You're looking for something called js.exe., which should be in the same directory.
In case someone is interested, Jim told me to build SpiderMonkey as explained in https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation.

Then any of the following works

    ./js/src/jit-test/jit_test.py ./js/src/build_OPT.OBJ/dist/bin/js.exe Object-proxy.js
    ./js/src/build_OPT.OBJ/dist/bin/js.exe js/src/jit-test/tests/debug/Object-proxy.js

Or, to run all the jit-test,

    ./js/src/jit-test/jit_test.py ./js/src/build_OPT.OBJ/dist/bin/js.exe
Attachment #8772475 - Attachment is obsolete: true
Attachment #8772591 - Flags: review?(jimb)
Comment on attachment 8772591 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object

Review of attachment 8772591 [details] [diff] [review]:
-----------------------------------------------------------------

Beautiful!
Attachment #8772591 - Flags: review?(jimb) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5000cae87e9
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object. r=jimb
Keywords: checkin-needed
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=32178100&repo=mozilla-inbound
Flags: needinfo?(oriol-bugzilla)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5fc76550ed
Backed out changeset b5000cae87e9 for bustage
Strange, the patch compiled for me. But I think the issue is that I used

    MOZ_ASSERT(isProxy());

instead of

    MOZ_ASSERT(object->isProxy());
Attachment #8772591 - Attachment is obsolete: true
Flags: needinfo?(oriol-bugzilla)
Attachment #8772948 - Flags: review?(jimb)
Oh, I see what happened here. You can see the results from running the tests after your patch landed here:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f4320d509686d6ef91ad4f5fff485adbbad18933

(I gather they land things in batches, so it's not your commit, but your commit must have been earlier.)

For each platform ("Linux opt"; "Windows PGO debug"), it starts with a build, "B", and then proceeds to run all the tests ("Cpp", "GTest", etc.). If you look, you'll notice that all the "debug" builds failed.

The reason your code compiled for you is probably that you are not using a debug build, so MOZ_ASSERT is #defined to do nothing. You were never compiling those "isProxy" calls at all.

When you're hacking on SpiderMonkey's C++ code, you should definitely be using a debug build. SpiderMonkey includes many, many assertions which will catch bugs for you much earlier.

The page you linked to in comment 27 includes instructions on how to do a debug build.
Comment on attachment 8772948 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object

Review of attachment 8772948 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Debugger.cpp
@@ +9696,5 @@
> +/* static */ bool
> +DebuggerObject::proxyTarget(JSContext* cx, Handle<DebuggerObject*> object,
> +                            MutableHandle<DebuggerObject*> result)
> +{
> +    MOZ_ASSERT(object->isProxy());

Yes, this looks better.
Attachment #8772948 - Flags: review?(jimb) → review+
OK, thanks for the explanation.

By the way, I just noticed there is a GetScriptedProxyHandlerObject defined in https://dxr.mozilla.org/mozilla-central/source/js/src/proxy/ScriptedProxyHandler.cpp#130

Probably, I should use that instead of

    js::GetProxyExtra(referent, 0).toObjectOrNull()

However, I don't know how to access that function. js::GetScriptedProxyHandlerObject does not work.

Should I include ScriptedProxyHandler.h or something?
This is the same patch, but using GetScriptedProxyHandlerObject from ScriptedProxyHandler.

I had to make that method public, and include the file.

Not sure if it's worth, but it seems it's more proper this way.
Attachment #8773104 - Flags: review?(jimb)
Comment on attachment 8773104 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object (using ScriptedProxyHandler)

Review of attachment 8773104 [details] [diff] [review]:
-----------------------------------------------------------------

It certainly would be better to use a method of ScriptedProxyHandler to retrieve the handler object, than to hard-code the GetProxyExtra index. I have to admit I didn't really notice that the first time I'd reviewed.

The code looks fine; r- only because I'm suggesting some renames and style changes. I'll try to review the revised patch promptly.

::: js/src/proxy/ScriptedProxyHandler.cpp
@@ +125,5 @@
>      return true;
>  }
>  
>  // Get the [[ProxyHandler]] of a scripted proxy.
> +JSObject*

There are a bunch of conventions that apply here:

- Definitions of static methods need to have /* static */ before the return type.

- Names starting with capital letters are generally reserved for global functions and type names; it seems like most static methods have names starting with lower-case letters.

- "get" is usually reserved for things that may fail; functions that either do their job or crash (i.e. assert) are just named after the thing they're accessing.

Putting those changes together, I think we end up with:

    /* static */ JSObject*
    ScriptedProxyHandler::handlerObject(...)

::: js/src/vm/Debugger.cpp
@@ +9032,5 @@
>      return obj->is<PromiseObject>();
>  }
>  
> +bool
> +DebuggerObject::isProxy() const

I think the DebuggerObject C++ APIs should actually be called "isScriptedProxy", "scriptedProxyTarget", and "scriptedProxyHandlerObject", but the JavaScript APIs and the C++ functions that implement them should keep the names they have now.

Since this isn't something I asked for in the original review, I should explain my thinking a bit:

In looking around the proxy code to understand the context of the getter function you're making public, I was reminded that the word "proxy" has an ambiguity in the C++ context that it lacks in the JavaScript context. In the JS world, there's only one kind of proxy: "proxy" always means "scripted proxy".

But in SpiderMonkey's C++ world, there are many different kinds of proxy. "Proxy" appearing in type names and function names without further qualification means any kind of proxy: cross-compartment wrappers, scripted proxies, etc. When one does mean a scripted proxy, it's called out explicitly.

In this context, it's kind of weird to have DebuggerObject::isProxy, which is intended for use from C++, be a different predicate on the D.O referent than js::IsProxy.

So given the different contexts, I think it makes sense for the JS APIs to continue to be named Debuggeer.Object.prototype.isProxy, proxyTarget, and proxyHandler, as they are now. And following convention, their native implementations should be called DebuggerObject::isProxyGetter, proxyTargetGetter, and proxyHandlerGetter, as they are now. But the C++ debugger API methods should follow C++ rules, and be called DebuggerObject::isScriptedProxy, scriptedProxyTarget, and scriptedProxyHandlerObject.

@@ +9712,5 @@
> +{
> +    MOZ_ASSERT(object->isProxy());
> +    RootedObject referent(cx, object->referent());
> +    Debugger* dbg = object->owner();
> +    RootedObject unwrapped(cx, ScriptedProxyHandler::GetScriptedProxyHandlerObject(referent));

Yeah, this is a lot better.
Attachment #8773104 - Flags: review?(jimb) → review-
Why scriptedProxyTarget and scriptedProxyHandlerObject?

Wouldn't either scriptedProxyTargetObject and scriptedProxyHandlerObject, or scriptedProxyTarget and scriptedProxyHandler be more coherent?
Depends on: 1288515
I have moved the changes in ScriptedProxyHandler to bug 1288515, which should land first.

So this patch has only the debugger-related changes.
Attachment #8772948 - Attachment is obsolete: true
Attachment #8773104 - Attachment is obsolete: true
Attachment #8773452 - Flags: review?(jimb)
(In reply to Oriol from comment #38)
> Why scriptedProxyTarget and scriptedProxyHandlerObject?
> 
> Wouldn't either scriptedProxyTargetObject and scriptedProxyHandlerObject, or
> scriptedProxyTarget and scriptedProxyHandler be more coherent?

Sorry, I was looking at the wrong code while writing. Yes, this is better.
Comment on attachment 8773452 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object (using ScriptedProxyHandler)

Review of attachment 8773452 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there...
- I want to take your suggestion in comment 38.
- The doc changes aren't correct.

::: js/src/doc/Debugger/Debugger.Object.md
@@ +189,5 @@
>      `undefined`.
>  
> +`isScriptedProxy`
> +:   If the referent is a (scripted) proxy, return `true`. If the referent is not
> +    a (scripted) proxy, return `false`.

These should be the JS API names, `isProxy`, `proxyTarget`, `proxyHandler`, not the C++ API names with `scripted` in them.

@@ +199,2 @@
>  
> +`scriptedProxyHandlerObject`

As you pointed out, this is better named `proxyHandler`, no `Object`.

::: js/src/vm/Debugger.cpp
@@ +9795,5 @@
> +    return dbg->wrapDebuggeeObject(cx, unwrapped, result);
> +}
> +
> +/* static */ bool
> +DebuggerObject::scriptedProxyHandlerObject(JSContext* cx, Handle<DebuggerObject*> object,

As you pointed out, this is better as `scriptedProxyHandler`, no `Object`.
Attachment #8773452 - Flags: review?(jimb) → review-
Fixing docs and scriptedProxyHandlerObject
Attachment #8773452 - Attachment is obsolete: true
Attachment #8773867 - Flags: review?(jimb)
Oh, and moving DebuggerObject::isScriptedProxy a bit upwards with the other infallible properties, now that isPromise is no longer one of these.
Attachment #8773867 - Attachment is obsolete: true
Attachment #8773867 - Flags: review?(jimb)
Attachment #8773869 - Flags: review?(jimb)
Comment on attachment 8773869 [details] [diff] [review]
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object (using ScriptedProxyHandler)

Review of attachment 8773869 [details] [diff] [review]:
-----------------------------------------------------------------

Looks perfect.
Attachment #8773869 - Flags: review?(jimb) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af78c13c85e5
Create accessor properties in Debugger.Object.prototype to expose the target and the handler of a proxy object. r=jimb
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd089ab2eada
Fix include ordering to make the style checker happy. r=bustage
https://hg.mozilla.org/mozilla-central/rev/af78c13c85e5
https://hg.mozilla.org/mozilla-central/rev/cd089ab2eada
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1291011
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: