See bug 820878.

WIP that incorporates some of jorendorff's ideas, but that doesn't provide any improvement over the current behavior. Going to look into it some more tomorrow.

My testing goes like this: 1) visit http://htmlpad.org/debugger 2) Click 'click me' 3) expand window.document.__proto__ in the variables view I'm looking at document.bgColor and document.baseURI mainly. For the former, getOwnPropertyDescriptor returns: {configurable:true, enumerable:true, get:{}, set:{}} and for the latter: {configurable:true, enumerable:true, get:{}, set:(void 0)} So the question here is whether we can find a workaround that will call the native getter and return the actual value.

I think Jason's brilliant idea to use obj.getOwnPropertyDescriptor("bgColor").get.call(obj) throws, because it tries to call the getter on the document prototype, instead of the document object. My theory is that this is another manifestation of bug 788148 and friends (bug 520882, bug 830828). I don't know how to test that theory and my only idea of a workaround so far is Jim's comment about using hostAnnotations: https://bugzilla.mozilla.org/show_bug.cgi?id=788148#c3

As discussed with bz, the problem stems from the way WebIDL attributes work, which is mandated by the spec IIUC. So, in this bug I'll also display those properties in the instance, as well as the prototype. bz suggested that there doesn't seem to be a non-hacky way to do this without a C++ API, but I'll try that first in order to get something working faster, since this is blocking the web console refactoring. Ultimately, hostAnnotations or a similar API (windowUtils.isWebIDLObject(obj) is another idea) will be the right fix. Another approach I'm following in my WIP is to not expose any of this to the client, or put another way, not require the client to send the parent object when requesting prototypeAndProperties. I think that will make the future fix easier to implement.

There is hope after all: document properties now appear on the instance with the values as expected. Still lots to do.

This version works even better. I managed to keep the client oblivious to the intricate details of how native getters are handled, by taking advantage of the fact that the user will hit this problem after expanding a subtree of the variables view. I keep around a cache of the prototype chains of all objects that have been visited through prototypeAndProperties requests, which are triggered when the user expands an object. When I come across a native getter I try calling it with |this| pointing to the current object, and if that throws, I walk backwards the scope chain trying to find an object that will not throw when used as |this| for the getter. I chose not to transplant these properties from the prototype to the instance, because there are cases where it is not that clear cut (c.f. the prototype chain of a 'p' element) and also because bz suggested that this is spec-mandated behavior, so it seems like a good tool would rather educate than placate. There are still some corner cases I want to verify and a few optimizations I want to make, plus the fun new tests I have to write. I think that this can work until we have a C++ API like hostAnnotations, preferably with a reference from the prototypes to the objects these getters actualy use.

(In reply to Panos Astithas [:past] from comment #8) > Created attachment 710286 [details] [diff] [review] > WIP 3 > > I chose not to transplant these properties from the prototype to the > instance, because there are cases where it is not that clear cut (c.f. the > prototype chain of a 'p' element) and also because bz suggested that this is > spec-mandated behavior, so it seems like a good tool would rather educate > than placate. > I like this behavior. However, I would vote for automatically expanding __proto__s when those are the only thing returned by a getPrototypeAndProperties request. What do you think?

(In reply to Victor Porof [:vp] from comment #9) > (In reply to Panos Astithas [:past] from comment #8) > > Created attachment 710286 [details] [diff] [review] > > WIP 3 > > > > I chose not to transplant these properties from the prototype to the > > instance, because there are cases where it is not that clear cut (c.f. the > > prototype chain of a 'p' element) and also because bz suggested that this is > > spec-mandated behavior, so it seems like a good tool would rather educate > > than placate. > > > > I like this behavior. However, I would vote for automatically expanding > __proto__s when those are the only thing returned by a > getPrototypeAndProperties request. What do you think? How far would such a behavior go if an object's __proto__ only contains a __proto__, etc.? I've come across such a case today: expand the |pAsProto| variable in our beloved test page.

(In reply to Panos Astithas [:past] from comment #10) > > How far would such a behavior go if an object's __proto__ only contains a > __proto__, etc.? I've come across such a case today: expand the |pAsProto| > variable in our beloved test page. I was thinking until the expanded __proto__ doesn't only contain a __proto__. Example: 1. expand pAsProto [object Object] -> only the __proto__ available for the variable 2. continue expanding -> only the __proto__ available for [object HTMLParagraphElement] 3. continue expanding -> multiple properties available for [object HTMLParagraphElementPrototype] 4. stop expanding

My only qualm is that it could make expanding a node feel janky due to the extra work and protocol traffic. If we can make it snappy in a remote debugging scenario (or at least not obviously worse than the current behavior) I'm with you on this.

All tests pass with this version, although I don't understand a test fix I had to make. Victor I might need your help here. Still not finished though.

(In reply to Panos Astithas [:past] from comment #13) > Created attachment 710953 [details] [diff] [review] > WIP 4 > Is it just me or does this patch not work? :) The previous version showed the values returned by native getter properly. > All tests pass with this version, although I don't understand a test fix I > had to make. Victor I might need your help here. Still not finished though. May be related to bug 837052, but I'm not sure.

(In reply to Victor Porof [:vp] from comment #14) > (In reply to Panos Astithas [:past] from comment #13) > > Created attachment 710953 [details] [diff] [review] > > WIP 4 > > > > Is it just me or does this patch not work? :) The previous version showed > the values returned by native getter properly. No, it's not you, I posted the patch with some last-minute changes that I hadn't properly tested. I've since fixed the problem and I'll post an updated patch later today. > > All tests pass with this version, although I don't understand a test fix I > > had to make. Victor I might need your help here. Still not finished though. > > May be related to bug 837052, but I'm not sure. OK, that's useful, I'll look into it some more.

(In reply to Panos Astithas [:past] from comment #15) > > No, it's not you, I posted the patch with some last-minute changes that I > hadn't properly tested. I've since fixed the problem and I'll post an > updated patch later today. > Thanks! I'll take a look at it again then.

(In reply to Victor Porof [:vp] from comment #16) Hmm, aArg is undefined at that point of the test. I've had experiences when undefined values would return as having a { get: undefined, set: undefined } descriptor, not with a plain grip as { value: undefined }. I suspect that may also be the reason you get 2 more delete buttons (one for the variable, one for the getter and one for the setter), but the variables view should have been smart enough to filter out such cases and just simply show aArg: undefined.

I'm reasonably happy with this version, but the test needs some more work. All tests pass with the patch as it is.

Victor, can you see if you can find a way to break this? All my testing works OK and all tests pass. I also made an extra change that makes bug 788148 more evident: instead of silently ignoring properties for which getting the descriptor throws, we now display the error as the value of the property. Failing silently is more often than not bad, especially for tools that cater to experts, and this way at least the user has an idea of what happened. Let me know how you feel about this, too.

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=14cb3130d1f9

Comment on attachment 711772 [details] [diff] [review] Patch v6 Review of attachment 711772 [details] [diff] [review]: ----------------------------------------------------------------- Love this! I couldn't find any way of breaking it, however (not for the scope of this bug) changing the values returned by those native getters doesn't seem to work, because you'll end up sending > "expression": "this[\"window\"][\"document\"][\"__proto__\"][\"title\"]=\"42\"" for client evaluation. I'm not entirely sure how we can circumvent this (maybe .replace(/\["__proto__"\]/g, ""), but I think that would have nasty side effects in case you'd actually want to modify something on the __proto__). Informing the frontend about the fact that "this is a value returned by a native getter" would be best, but will it work in all cases? Does it always mean that we should bypass __proto__? I'll file the bug to automatically expand __proto__s until they're not void of properties or [object Object] is reached. ::: toolkit/devtools/debugger/server/dbg-script-actors.js @@ +1686,5 @@ > + desc = this.obj.getOwnPropertyDescriptor(aName); > + } catch (e) { > + // Calling getOwnPropertyDescriptor on wrapped native prototypes is not > + // allowed (bug 560072). Inform the user with a bogus, but hopefully > + // explanatory, descriptor. This is fine, however you may end up with something like the screenshot attached. I'm not sure if "NS_ERROR_XPC_BAD_OP_ON_WN_PROTO" is terribly informative for the end user, maybe we can think of something more meaningful to show, like a "?" icon, or...? @@ +1717,5 @@ > + for (let i = index; i >= 0; i--) { > + // XXX: limit the application of this algorithm to known object > + // classes? Do we care what side-effects a native-plugin-provided > + // getter has? Create a class whitelist from the WebIDL spec plus > + // any XPCOM classes of ours? It would just be a tin plate security layer, since creating my own (for example) "function Event() {}" is not that unlikely to happen in the wild. @@ +1721,5 @@ > + // any XPCOM classes of ours? > + // if (chain[i] && (chain[i].class.startsWith("XPC") || > + // chain[i].class.startsWith("HTML")...)) { > + // If we had hostAnnotations we would have been able to filter > + // on chain[i].hostAnnotations.isWebIDLObject or similar. Yes, waiting for bug 801084 is probably the smartest thing to do.

Comment on attachment 711772 [details] [diff] [review] Patch v6 (In reply to Victor Porof [:vp] from comment #21) > Love this! I couldn't find any way of breaking it, however (not for the > scope of this bug) changing the values returned by those native getters > doesn't seem to work, because you'll end up sending > > "expression": "this[\"window\"][\"document\"][\"__proto__\"][\"title\"]=\"42\"" > for client evaluation. I'm not entirely sure how we can circumvent this > (maybe .replace(/\["__proto__"\]/g, ""), but I think that would have nasty > side effects in case you'd actually want to modify something on the > __proto__). Informing the frontend about the fact that "this is a value > returned by a native getter" would be best, but will it work in all cases? > Does it always mean that we should bypass __proto__? That's a tricky case indeed. The only hint that the expression we are trying to eval should fall under this special treatment, is the error type being NS_ERROR_XPC_BAD_OP_ON_WN_PROTO. And that will not always be the case according to the WebIDLs spec: http://dev.w3.org/2006/webapi/WebIDL/#es-attributes See section 3.3 there, and also bz's comments in bug 790303. I'm always wary of software that tries to be too smart, and we will very soon have the ability to make such changes from the web console, so doing nothing strikes me as the right balance here. > ::: toolkit/devtools/debugger/server/dbg-script-actors.js > @@ +1686,5 @@ > > + desc = this.obj.getOwnPropertyDescriptor(aName); > > + } catch (e) { > > + // Calling getOwnPropertyDescriptor on wrapped native prototypes is not > > + // allowed (bug 560072). Inform the user with a bogus, but hopefully > > + // explanatory, descriptor. > > This is fine, however you may end up with something like the screenshot > attached. I'm not sure if "NS_ERROR_XPC_BAD_OP_ON_WN_PROTO" is terribly > informative for the end user, maybe we can think of something more > meaningful to show, like a "?" icon, or...? Clearly it's not very obvious, but it is google-able, and I was hoping it might exert peer pressure to fix the underlying issue. In other words, I can see how a question mark or an "Oops!" label may indicate a system error, but this is a known system error, so I was thinking it makes sense to differentiate. > @@ +1717,5 @@ > > + for (let i = index; i >= 0; i--) { > > + // XXX: limit the application of this algorithm to known object > > + // classes? Do we care what side-effects a native-plugin-provided > > + // getter has? Create a class whitelist from the WebIDL spec plus > > + // any XPCOM classes of ours? > > It would just be a tin plate security layer, since creating my own (for > example) "function Event() {}" is not that unlikely to happen in the wild. I don't think such cases would get this far, since the Debuger.Object instances corresponding to these functions would have Debugger.Script references in them, so "fn.script !== undefined", but... > @@ +1721,5 @@ > > + // any XPCOM classes of ours? > > + // if (chain[i] && (chain[i].class.startsWith("XPC") || > > + // chain[i].class.startsWith("HTML")...)) { > > + // If we had hostAnnotations we would have been able to filter > > + // on chain[i].hostAnnotations.isWebIDLObject or similar. > > Yes, waiting for bug 801084 is probably the smartest thing to do. ...having said that, I think you're right about this. Since we're making all sorts of judgement calls here, let's get Rob's opinion, too.

Comment on attachment 711772 [details] [diff] [review] Patch v6 Review of attachment 711772 [details] [diff] [review]: ----------------------------------------------------------------- That is quite the sordid story in this bug and I agree with where you ended up on it. Let's not make our tools overly clever and go as far as we need to satisfy the intent of this bug. The solution looks pretty clean as it is here. I'm happy with filing further follow-ups to take care of any additional cleanup.

Bug 839179 bitrotted the test, so I had to fix it. Landed: https://hg.mozilla.org/integration/fx-team/rev/c4fc63d7317b