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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: Oriol, Assigned: Oriol)
References
Details
Attachments
(1 file, 10 obsolete files)
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 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
Agreed that this should be on Debugger.Object.
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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-
Comment 6•8 years ago
|
||
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.)
Assignee | ||
Comment 7•8 years ago
|
||
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
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
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.
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
(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.
Comment 14•8 years ago
|
||
(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 15•8 years ago
|
||
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 16•8 years ago
|
||
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.
Assignee | ||
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
(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 19•8 years ago
|
||
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-
Assignee | ||
Comment 20•8 years ago
|
||
(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 21•8 years ago
|
||
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+
Comment 22•8 years ago
|
||
Your test code looks totally fine to me. Can you tell me exactly how you're invoking js/src/jit-test/jit_test.py?
Assignee | ||
Comment 23•8 years ago
|
||
(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).
Comment 24•8 years ago
|
||
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.
Comment 25•8 years ago
|
||
BTW, you can find me on Mozilla IRC as 'jimb' in #jsapi.
Comment 26•8 years ago
|
||
(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.
Assignee | ||
Comment 27•8 years ago
|
||
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 28•8 years ago
|
||
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+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 29•8 years ago
|
||
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
Comment 30•8 years ago
|
||
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)
Comment 31•8 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5fc76550ed Backed out changeset b5000cae87e9 for bustage
Assignee | ||
Comment 32•8 years ago
|
||
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)
Comment 33•8 years ago
|
||
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 34•8 years ago
|
||
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+
Assignee | ||
Comment 35•8 years ago
|
||
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?
Assignee | ||
Comment 36•8 years ago
|
||
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 37•8 years ago
|
||
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-
Assignee | ||
Comment 38•8 years ago
|
||
Why scriptedProxyTarget and scriptedProxyHandlerObject? Wouldn't either scriptedProxyTargetObject and scriptedProxyHandlerObject, or scriptedProxyTarget and scriptedProxyHandler be more coherent?
Assignee | ||
Comment 39•8 years ago
|
||
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)
Comment 40•8 years ago
|
||
(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 41•8 years ago
|
||
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-
Assignee | ||
Comment 42•8 years ago
|
||
Fixing docs and scriptedProxyHandlerObject
Attachment #8773452 -
Attachment is obsolete: true
Attachment #8773867 -
Flags: review?(jimb)
Assignee | ||
Comment 43•8 years ago
|
||
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 44•8 years ago
|
||
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+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 45•8 years ago
|
||
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
Comment 46•8 years ago
|
||
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
Comment 47•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af78c13c85e5 https://hg.mozilla.org/mozilla-central/rev/cd089ab2eada
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•