STR: 1. Open Developer Tools' Web Console 2. At the prompt, enter: navigator.plugins 3. In the console, click the "[object PluginArray]" that should appear. 4. In the object properties panel, expand the "[object Plugin]" properties' "__proto__" properties. RESULT: All the __proto__ properties' properties (e.g. "name: Shockwave Flash") are duplicates of the first "[object Plugin]" that was expanded. The bug seems to be somewhat intermittent. I can reproduce it on my first attempt about 90% of the time. If the first attempt doesn't fail, then a second attempt with "navigator.mimeTypes" seems to always fail. I believe this is a regression in Nightly 2013-04-10, when the Web Console's new property inspector landed. Here is the pushlog from 04-09 to 04-10: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=ee5ca214e87c

Panos, any thoughts on this? Sounds like a problem related to bug 830818.

Investigated the issue. I can confirm it happens. Thanks Chris for the bug report. I also looked into why this happens and it seems the onPrototypeAndProperties() method of the ObjectActor doesn't take into account the possibility of different objects sharing the same prototype chain. For example: navigator.plugins[0].__proto__ == navigator.plugins[1].__proto__ is |true|. Once you expand plugins[0] the next plugin you expand will always find the first-expanded object. This bug is easy to reproduce in other cases as well: try $$("p") on any page with several paragraphs, then expand two __proto__s deep and see offsetTop/offsetHeight, for several paragraphs. You will see that all paragraphs show the same offsets, same properties.

Spent more time to determine what's going on and how we could fix this bug. It looks to me like we need to have some kind of parent-child relationship, such that when we make the request for prototypeAndProperties to an object we also tell it's "parent" - this would give us the right behavior. Another approach, without changing the protocol, is to further extend our current server-side hack: when you walk the properties of an object instead of doing a simple call to createValueGrip(), which gives one unique ObjectActor (with a Debugger.Object), for each child object... we could also integrate the knowledge of ownership. This would allow us to have two different ObjectActor instances for the same Debugger.Object that wraps the same __proto__, but with a different owner. OA's would have a new .owner property pointing to some "parent" OA that created it. This could be uglier than a protocol change, but it would provide us with the same relationship information that we need when displaying properties. Any thoughts?

(In reply to Mihai Sucan [:msucan] from comment #3) > Spent more time to determine what's going on and how we could fix this bug. > > It looks to me like we need to have some kind of parent-child relationship, > such that when we make the request for prototypeAndProperties to an object > we also tell it's "parent" - this would give us the right behavior. Another > approach, without changing the protocol, is to further extend our current > server-side hack: when you walk the properties of an object instead of doing > a simple call to createValueGrip(), which gives one unique ObjectActor (with > a Debugger.Object), for each child object... we could also integrate the > knowledge of ownership. This would allow us to have two different > ObjectActor instances for the same Debugger.Object that wraps the same > __proto__, but with a different owner. OA's would have a new .owner property > pointing to some "parent" OA that created it. This could be uglier than a > protocol change, but it would provide us with the same relationship > information that we need when displaying properties. > > Any thoughts? This is, I think, a bug in our specification for how we deal with ObjectActors and prototype chains. I don't think extending the hack is the right approach here. We should fix how we access prototypeAndProperties. i.e., let's do it right and not extend hacks into overly complex implementations.

cc'ing jim for feedback on this as well.

I would prefer a protocol change as well. We could remove the current hacks.

Why do we need to show values on prototypes? Why not show the native getters as they were before bug 830818. We can revert those changes, drop any hacks and avoid the parent-child relationship thing I suggested above. Instead of showing properties on prototypes what we can do is add a new request type for ObjectActor, something like "inspectObject" that iterates through the object's properties and methods, flattening the prototype chains. We can show values for native getters, without needing to expand the .prototype/__proto__ object or go through chains of protos to see some DOM element property value. At the end of the list we can show __proto__ and let users go down through the prototype chain as they want, without forcing evaluation of native getters *on the prototype* (like we do now). Wouldn't this be a better approach?

I... don't understand what onPrototypeAndProperties is doing.

Mihai, Dave and I discussed possible solutions on IRC; we decided to provide values available via inherited safe getters directly in the "prototypeAndProperties" reply packet for the object that inherits the getters. A draft spec for the protocol changes is here: https://github.com/jimblandy/DebuggerDocs/blob/safe-getters/protocol I'll also attach it as a diff, since github doesn't handle long lines well. Here's the algorithm we settled on for implementing the specified behavior efficiently: First, define a 'safe getter' as one which can be applied to appropriate instances without side effects. We have no solid way to recognize these, but assuming that native functions appearing as getter functions are safe to call at any time seems to be an adequate heuristic. Next, define a 'getter applicable to O' as a getter function that doesn't throw when called with O as its 'this' value, and no arguments. We want to find getters that meet both requirements: safe, applicable getters. Every "prototypeAndProperties" request for an object O should search O itself and O's prototype chain for safe getters applicable to O. For each object P (typically on O's prototype chain, but possibly O itself), we should set a property on the Debugger.Object referring to P whose value is an array (or Set?) of the names of safe getters P owns. We should set this property on P's D.O even if P owns no safe getters, to an empty array (or Set). If we encounter a P which already has such a property, we'll know we've already searched P and its prototype chain for safe applicable getters, and we needn't do so again. Then, if O inherits any safe, applicable getters, we will apply them to produce a "safeGetterValues" property to include in the "prototypeAndProperties" reply, as described in the spec. This algorithm assumes that safe getters will be equally applicable to any object that inherits them: the first encounter is enough to assign them "applicable" status. This is certainly not true in general --- any object can use MouseEvent.prototype as its prototype --- but as long as we're prepared for safe getters to throw, we should be all right. The UI will need to check for this property and display the properties appropriately, in a way that distinguishes them from true "own" properties. The heuristic ObjectActor.prototype._propertyDescriptor now uses to recognize applicable safe getters is to assume that any native function appearing as a getter is safe: let fn = desc.get; if (fn && fn.callable && fn.class == "Function" && fn.script === undefined) { ... It then tries to apply the getter, and sees whether it throws or not, to decide if it is applicable.

Here's a suggested protocol change spec.

Thank you Jim. I'm working on a patch. Will submit it once it's ready.

This patch has everything working as intended and all tests pass. I still need to write one or two new tests that check we fix the problem reported in this bug. I am asking Victor for review because I also did changes to Variables View as discussed with dcamp and jimb: we need to suggest visually that properties are native/safe getters. Jim: thank you for the updated protocol specification. I did, however, stray a bit from it: safe getter descriptors should include the 'enumerable' and 'writable' flags. These are used by the variables view and providing these flags to the view would otherwise be guess work. Your reviews and feedback are much appreciated. Thank you!

(In reply to Mihai Sucan [:msucan] from comment #12) > Created attachment 748130 [details] [diff] [review] > wip1 > Jim: thank you for the updated protocol specification. I did, however, stray > a bit from it: safe getter descriptors should include the 'enumerable' and > 'writable' flags. These are used by the variables view and providing these > flags to the view would otherwise be guess work. Do we need to send up a documentation change for these?

Comment on attachment 748130 [details] [diff] [review] wip1 Review of attachment 748130 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/webconsole/webconsole.js @@ +3522,5 @@ > + } > + else { > + ownProperties[name] = safeGetterValues[name]; > + } > + } be nice when these get refactored. ::: toolkit/devtools/Console.jsm @@ +186,5 @@ > reply += logProperty('' + i, value); > i++; > } > } > + else if (type.match("Error$") || aThing.name == "NS_ERROR_FAILURE") { I hope this isn't getting truncated or someone will have an aneurysm. ::: toolkit/devtools/debugger/server/dbg-script-actors.js @@ +1546,5 @@ > + try { > + desc = obj.getOwnPropertyDescriptor(name); > + getter = desc.get; > + } catch (ex) { > + // The above can throw if the cache becomes stale. hiding errors. tsk.

Comment on attachment 748130 [details] [diff] [review] wip1 Review of attachment 748130 [details] [diff] [review]: ----------------------------------------------------------------- Lovely stuff, kudos! r+ with the comments addressed, but keep in mind that I only reviewed VariablesView, Widgets and Debugger code. I just glanced over console changes and they pretty much look ok. ::: browser/devtools/debugger/test/browser_dbg_propertyview-11.js @@ -216,5 @@ > - "The buttonAsProtoProtoProtoNode should be expanded at this point."); > - > - // Poll every few milliseconds until the properties are retrieved. > - // It's important to set the timer in the chrome window, because the > - // content window timers are disabled while the debuggee is paused. I would move this into a generic function outside, just like in other tests. Makes things more readable. ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-01.js @@ +152,5 @@ > is(globalScope.querySelectorAll(".variables-view-variable:not([non-match])").length, 1, > "There should be 1 variable displayed in the global scope"); > > + ok(innerScope.querySelectorAll(".variables-view-property:not([non-match])").length > 6, > + "There should be more than 6 properties displayed in the inner scope"); Turning this from "is 6" to "is at least 6" makes me nervous (here and every other test). Let's test for an exact number please. @@ +178,2 @@ > is(innerScope.querySelectorAll(".variables-view-property:not([non-match]) > .title > .name")[5].getAttribute("value"), > + "hash", "The sixth inner property displayed should be 'hash'"); ..and here, test for all the other properties (how many more than 6 are they?). There should be a very specific variables view structure when filtering, and we need to make sure of this. @@ +240,5 @@ > is(globalScope.querySelectorAll(".variables-view-variable:not([non-match])").length, 1, > "There should be 1 variable displayed in the global scope"); > > + ok(innerScope.querySelectorAll(".variables-view-property:not([non-match])").length > 6, > + "There should be more than 6 properties displayed in the inner scope"); Ditto. @@ +266,2 @@ > is(innerScope.querySelectorAll(".variables-view-property:not([non-match]) > .title > .name")[5].getAttribute("value"), > + "hash", "The sixth inner property displayed should be 'hash'"); More checks here please. The fact that "hash" is displayed when filtering for "location" can only happen if a node containing "location" is a child of "hash", we need to make sure this is the case. ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-02.js @@ +27,5 @@ > gDebugger.DebuggerController.StackFrames.autoScopeExpand = true; > gDebugger.DebuggerView.Variables.delayedSearch = false; > + gDebugger.DebuggerView.Variables.lazyAppend = false; > + gDebugger.DebuggerView.Variables.lazyExpand = false; > + gDebugger.DebuggerView.Variables.lazyEmpty = false; This is already done in head.js, no need to repeat ourselves :) @@ +95,5 @@ > is(globalScope.querySelectorAll(".variables-view-variable:not([non-match])").length, 2, > "There should be 2 variables displayed in the global scope"); > > + ok(innerScope.querySelectorAll(".variables-view-property:not([non-match])").length > 8, > + "There should be more than 8 properties displayed in the inner scope"); Ditto for testing an exact number. @@ +125,2 @@ > is(innerScope.querySelectorAll(".variables-view-property:not([non-match]) > .title > .name")[7].getAttribute("value"), > + "applets", "The eight inner property displayed should be 'applets'"); You know what I'm going to say :) @@ +171,5 @@ > "The local scope 'this.window' should be expanded"); > is(documentItem.expanded, true, > "The local scope 'this.window.document' should be expanded"); > + is(locationItem.expanded, true, > + "The local scope 'this.window.document.location' should be expanded"); Why did this change? (just making sure) @@ +189,5 @@ > is(globalScope.querySelectorAll(".variables-view-variable:not([non-match])").length, 2, > "There should be 2 variables displayed in the global scope"); > > + ok(innerScope.querySelectorAll(".variables-view-property:not([non-match])").length > 8, > + "There should be more than 8 properties displayed in the inner scope"); Idem :) @@ +219,2 @@ > is(innerScope.querySelectorAll(".variables-view-property:not([non-match]) > .title > .name")[7].getAttribute("value"), > + "applets", "The eight inner property displayed should be 'applets'"); Double idem. ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-05.js @@ +127,5 @@ > "There should be 0 properties displayed in the math scope"); > is(testScope.querySelectorAll(".variables-view-property:not([non-match])").length, 0, > "There should be 0 properties displayed in the test scope"); > + ok(loadScope.querySelectorAll(".variables-view-property:not([non-match])").length > 1, > + "There should be more than one properties displayed in the load scope"); Wohoooo! Exact numbers are awesomee! @@ +157,5 @@ > "There should be 0 properties displayed in the math scope"); > is(testScope.querySelectorAll(".variables-view-property:not([non-match])").length, 0, > "There should be 0 properties displayed in the test scope"); > + ok(loadScope.querySelectorAll(".variables-view-property:not([non-match])").length > 1, > + "There should be more than one properties displayed in the load scope"); Chickens! ::: browser/devtools/shared/widgets/VariablesView.jsm @@ +2001,5 @@ > * - { value: { type: "null" } } > * - { value: { type: "object", class: "Object" } } > * - { get: { type: "object", class: "Function" }, > * set: { type: "undefined" } } > + * - { get: { ... }, getterValue: "foo", getterPrototypeLevel: 2 } I think it'd be a good idea to be explicit about what ... means. @@ +2430,5 @@ > } > if (!descriptor.null && !descriptor.writable && !this.ownerView.getter && !this.ownerView.setter) { > this._target.setAttribute("non-writable", ""); > } > + if (descriptor && "getterValue" in descriptor) { Descriptor will always be at least an empty object (there are default params everywhere), no need to check if it's null. @@ +2750,5 @@ > + if ("getterValue" in aDescriptor) { > + aDescriptor.value = aDescriptor.getterValue; > + delete aDescriptor.get; > + delete aDescriptor.set; > + } What happens if you have a getter on a global, like window? Wouldn't it make more sense to move this to a Variable and not special-case properties? setGrip would be a good place, I think? ::: browser/devtools/webconsole/webconsole.js @@ +3522,5 @@ > + } > + else { > + ownProperties[name] = safeGetterValues[name]; > + } > + } I really want that VariablesViewController! ::: browser/themes/linux/devtools/widgets.css @@ +543,5 @@ > text-decoration: line-through; > } > > +.variable-or-property:not([safe-getter]) > tooltip > label[value=native-getter] { > + display: none; Yup, I also think that always displaying this label in the tooltip is a bad idea, good thinking!

Comment on attachment 748130 [details] [diff] [review] wip1 Review of attachment 748130 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/linux/devtools/widgets.css @@ +506,5 @@ > border-bottom: 1px dashed #f99; > } > > +.variable-or-property[safe-getter] > .title > .name { > + border-bottom: 2px solid #8f8; I'd also make this 1px and a more darker green. It burns my eyes a bit as it is.

Fixed bugs, added comments, more test fixes, addressed Victor's comments* and added two web console tests for native getters (one in toolkit and another one in browser). * Discussed with Victor on IRC why it would be unrealistic to check every single property of the Document object in the debugger vview tests. We've agreed on some changes and this is the result. Try push: https://tbpl.mozilla.org/?tree=Try&rev=b25a6129ffbe This should be ready to land tomorrow.

Comment on attachment 748238 [details] [diff] [review] patch with tests Review of attachment 748238 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-01.js @@ +176,5 @@ > + "location", "The first global variable displayed should be 'location'"); > + is(globalScope.querySelectorAll(".variables-view-variable:not([non-match]) > .title > .name")[1].getAttribute("value"), > + "locationbar", "The second global variable displayed should be 'locationbar'"); > + is(globalScope.querySelectorAll(".variables-view-variable:not([non-match]) > .title > .name")[2].getAttribute("value"), > + "Location", "The second global variable displayed should be 'Location'"); s/second/third/ ::: browser/devtools/debugger/test/browser_dbg_propertyview-filter-05.js @@ +127,5 @@ > "There should be 0 properties displayed in the math scope"); > is(testScope.querySelectorAll(".variables-view-property:not([non-match])").length, 0, > "There should be 0 properties displayed in the test scope"); > + ok(loadScope.querySelectorAll(".variables-view-property:not([non-match])").length > 1, > + "There should be more than one properties displayed in the load scope"); s/properties/property/

Thank you Victor! That's what I got last evening being in a hurry and slightly tired. :) At least try push was green.

https://tbpl.mozilla.org/?tree=Fx-Team&rev=4dbd432e7667 https://hg.mozilla.org/integration/fx-team/rev/89dd69c8c5c8 Although: "CLOSED. BuiltBot issues: jobs don't seem to be showing up; tbpl fails to finish loading; retriggers not working;."

Comment on attachment 748407 [details] [diff] [review] fix typos Review of attachment 748407 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/debugger/server/dbg-script-actors.js @@ +1558,5 @@ > + desc = obj.getOwnPropertyDescriptor(name); > + getter = desc.get; > + } catch (ex) { > + // The above can throw if the cache becomes stale. > + } This is really here because of bug 560072, right? That's an unfortunate characteristic of Debugger.Object.prototype.getOwnPropertyDescriptor; poisonous properties shouldn't bother a function whose job it is to describe properties. It requires essentially every use of getOwnPropertyDescriptor to be wrapped like this, unless you somehow know for sure what kind of object you're pointing it at, which a debugger usually doesn't. Is there a bug filed against Debugger.Object.prototype.getOwnPropertyDescriptor for this? (I was going to grouse at you guys about this, but I should have had more faith in you; I don't see any way to avoid this.)

Fast work, Mihai!

(In reply to Mihai Sucan [:msucan] from comment #12) > Jim: thank you for the updated protocol specification. I did, however, stray > a bit from it: safe getter descriptors should include the 'enumerable' and > 'writable' flags. These are used by the variables view and providing these > flags to the view would otherwise be guess work. Yes, that's completely appropriate. I'll fix the docs, and land them.

(In reply to Jim Blandy :jimb from comment #22) > Is there a bug filed against > Debugger.Object.prototype.getOwnPropertyDescriptor for this? Filed as bug 871171.

It's only fitting I guess that such bugs are discovered when the culprit is away on vacation, Murphy's law still rules. Thank you all for looking into this! First, let me make a brief diversion to talk about hacks. The very existence of this code is a hack, since it is special-casing some object properties due to the way WebIDL attributes work. Therefore by definition we can't drop all the hacks and use a "clean" solution, since no such solution can remove the need for special handling. We need to use some sort of caching in order to maintain the stateless form of the prototypeAndProperties API. Granted, some forms of caching are better than others, and of course bugs are never acceptable, regardless of the hacky or clean style of the code :-) The bug stems from the fact that the code used to look for the current object in the prototype chain cache and reuse the entire cache line if it was found. That choice was made in order to fix the (corner) case of objects created with a DOM element as their prototype. I believe that fixing that lookup part would have been another way to fix this bug, but moving the cache into the object themselves like this patch does is cool, too. There is one little detail that I want to make sure we have considered though. The proto chain cache did not cache getter values, only the links between prototypes and their instances, so that a getter could be called on the right instance when requested. The way we now cache values means that the object could change state, without it being reflected in the value the server reports back to the client. Luckily Debugger.Object instances are pause-scoped in the debugger, so each time the value of a WebIDL attribute changes, the debuggee has been running and the next pause would generate a fresh Debugger.Object instance for the client to see. The web console code though uses long-lived Debugger.Object instances IIRC, so this could be an issue there. So does Firebug IIANM. Mihai and Honza would know best. I'm glad that moving the WebIDL attributes to the instance was done with a protocol change. In bug 830818 I had noted that displaying WebIDL attributes as properties on the prototype was a judgement call that I expected to stir some disagreement, but nobody seemed to mind. I'm aware of the pros of displaying those properties on the instance and I'm OK with changing that. Doing it in a way that does not hide from the debugger client what is actually happening, like this patch and spec change does, is definitely the best way to do it. I've noticed that the updated protocol spec mentions getterValue in one place and valueViaSafeGetter in another. I assume the former is a typo? Also, Mihai added enumerable and writable to the safe getter descriptors, but not configurable. Why not be thorough and consistent with expectations? Displaying "native-getter" in the property tooltip seems rather confusing, both because it's not a standard JS descriptor property and because web devs may not be familiar with how WebIDL attributes are implemented in browser engines (in "native" code, usually C++). Perhaps the former issue is not so bad, in which case we should probably add sealed/frozen/non-extensible there as well, in bug 760370. The latter may be remedied by renaming native-getter to WebIDL-attr or somesuch. There may exist a remote possibility that such native getters emulating WebIDL attribute behavior may be provided by plugins, but even so, calling them after what they emulate seems appropriate to me. Note that our current heuristic does not work for all kinds of native getters, like the ones that were present before the Paris-bindings effort (e.g. bug 560072). On that matter, the try/catch around getPropertyDescriptor now silently ignores NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, whereas it used to create a dummy descriptor with the error value before, as a means to educate about the problem (see bug 830818 comment 23). If that was a conscious decision then that's fine, just wanted to call that out. And finally one weird thing I observed was in the http://htmlpad.org./debugger/ test page. Clicking the 'Click me' button and inspecting the pAsProto variable in the debugger shows onmouseenter and onmouseleave properties on the instace but with undefined values. This seems wrong, since such event listeners on DOM elements have the value null when not set, which is indeed the case for pAsProto.__proto__. The same thing happens for the docAsProto variable as well. Phew, that's all I could think of! If I had more time I would have made a shorter comment :-)

There's never any shame in doing the best one could see how at the time --- and shipping while it still matters. :D There are still certainly some kludges here: 1) We assume that native getters have no side effects. I don't know enough about the web platform to know how true this actually is. 2) We assume that prototype objects' values don't change. While object actors are pause-scoped, the annotations we place on Debugger.Object instances will certainly persist from one pause to the next, so if someone changes the accessor properties on prototype objects, our cache will be out-of-date. (We could certainly make our cache paused-scoped, by using a WeakMap to store the D.O -> cache entry mapping, instead of a real property on the D.O, and tossing the WeakMap on each resume.) I think, though, that the protocol change addresses a real need; as the docs say, the use of inherited accessors is ubiquitous on the web, and not showing those values immediately was a usability hit. Thanks for the "valueViaSafeGetter" correction; I've fixed the docs and GitHub.

(In reply to Panos Astithas [:past] from comment #26) > It's only fitting I guess that such bugs are discovered when the culprit is > away on vacation, Murphy's law still rules. Thank you all for looking into > this! Thanks for your detailed testing and reply. > The proto chain cache did not cache getter values, only the links between > prototypes and their instances, so that a getter could be called on the > right instance when requested. The way we now cache values means that the > object could change state, without it being reflected in the value the > server reports back to the client. Luckily Debugger.Object instances are > pause-scoped in the debugger, so each time the value of a WebIDL attribute > changes, the debuggee has been running and the next pause would generate a > fresh Debugger.Object instance for the client to see. The web console code > though uses long-lived Debugger.Object instances IIRC, so this could be an > issue there. So does Firebug IIANM. Mihai and Honza would know best. That is correct, however this patch doesn't add getter value caching. We only cache the known "safe getters" for every Debugger.Object. > I'm glad that moving the WebIDL attributes to the instance was done with a > protocol change. In bug 830818 I had noted that displaying WebIDL attributes > as properties on the prototype was a judgement call that I expected to stir > some disagreement, but nobody seemed to mind. I'm aware of the pros of > displaying those properties on the instance and I'm OK with changing that. > Doing it in a way that does not hide from the debugger client what is > actually happening, like this patch and spec change does, is definitely the > best way to do it. Indeed, this approach feels less hacky that the previous one - it's still a hack of some sorts... especially in the way we check if "is this a safe getter?" > I've noticed that the updated protocol spec mentions getterValue in one > place and valueViaSafeGetter in another. I assume the former is a typo? I assumed the latter is a typo. Given an object safeGetterValues: { foo: { getterValue: "bar", getterPrototypeLevel: 3 } }, I believe it makes sense to use "getterValue" instead of the more-verbose "valueViaSafeGetter". Plus, the protocol draft that Jim posted (on github) used "getterValue" where safe getter descriptors were defined. > Also, Mihai added enumerable and writable to the safe getter descriptors, > but not configurable. Why not be thorough and consistent with expectations? I was torn about this. My thoughts: - safe getter descriptors seemed at the point of writing the code slightly under-defined. When you work with vview you need to know if the given property is enumerable - it does affect presentation. You get that from property descriptor of the getter on the prototype object where you find it. - writable is always true when prototype level > 0, from the perspective of the object to which the getter is applied: you can always override a property coming from a prototype, by adding an own-property to obj. Vview uses this flag as well. Now I believe the best way here would be to simply include all property descriptor flags from the prototype where the getter was found. > Displaying "native-getter" in the property tooltip seems rather confusing, > both because it's not a standard JS descriptor property and because web devs > may not be familiar with how WebIDL attributes are implemented in browser > engines (in "native" code, usually C++). Perhaps the former issue is not so > bad, in which case we should probably add sealed/frozen/non-extensible there > as well, in bug 760370. The latter may be remedied by renaming native-getter > to WebIDL-attr or somesuch. Ah, good point. "WebIDL-attr" is probably better. I didn't consider the tooltip a strict list of JS property descriptors, that's why I added that there - otherwise how would we inform the user that's some kind of different property? > There may exist a remote possibility that such native getters emulating > WebIDL attribute behavior may be provided by plugins, but even so, calling > them after what they emulate seems appropriate to me. Note that our current > heuristic does not work for all kinds of native getters, like the ones that > were present before the Paris-bindings effort (e.g. bug 560072). > > On that matter, the try/catch around getPropertyDescriptor now silently > ignores NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, whereas it used to create a dummy > descriptor with the error value before, as a means to educate about the > problem (see bug 830818 comment 23). If that was a conscious decision then > that's fine, just wanted to call that out. Any such property can now be found on its prototype, unaltered. Should we present it in safeGetterValues with getterValue showing the error value? > And finally one weird thing I observed was in the > http://htmlpad.org./debugger/ test page. Clicking the 'Click me' button and > inspecting the pAsProto variable in the debugger shows onmouseenter and > onmouseleave properties on the instace but with undefined values. This seems > wrong, since such event listeners on DOM elements have the value null when > not set, which is indeed the case for pAsProto.__proto__. The same thing > happens for the docAsProto variable as well. I also noticed that. it seems to happen because those properties are "safe getters" that can apply to |pAsProto|, not only to |p|. > Phew, that's all I could think of! If I had more time I would have made a > shorter comment :-) Thank you! :) Should we open a follow-up bug to clean things up? Maybe even get it in aurora.

(In reply to Mihai Sucan [:msucan] from comment #28) > > Also, Mihai added enumerable and writable to the safe getter descriptors, > > but not configurable. Why not be thorough and consistent with expectations? > > I was torn about this. My thoughts: > > - safe getter descriptors seemed at the point of writing the code slightly > under-defined. When you work with vview you need to know if the given > property is enumerable - it does affect presentation. You get that from > property descriptor of the getter on the prototype object where you find it. > > - writable is always true when prototype level > 0, from the perspective of > the object to which the getter is applied: you can always override a > property coming from a prototype, by adding an own-property to obj. Vview > uses this flag as well. > > Now I believe the best way here would be to simply include all property > descriptor flags from the prototype where the getter was found. I am not sure if I was clear enough: here I was torn because my initial intent was to keep protocol and patch changes minimal. The configurable property did not seem important enough. Also, if we add all the property descriptors one could also ask for the property setter and getter functions (their ObjectActor grips). Should we turn safe getter values descriptors into property descriptors with two added properties? (getterValue and getterPrototypeLevel)

(In reply to Mihai Sucan [:msucan] from comment #29) > I am not sure if I was clear enough: here I was torn because my initial > intent was to keep protocol and patch changes minimal. The configurable > property did not seem important enough. Also, if we add all the property > descriptors one could also ask for the property setter and getter functions > (their ObjectActor grips). Should we turn safe getter values descriptors > into property descriptors with two added properties? (getterValue and > getterPrototypeLevel) The configurable property is displayed in the tooltip, so these transplanted properties end up being the only ones with an unspecified configurable property. I don't think we need to send the getter/setter properties though: this new descriptor is the result of effectively converting an accessor descriptor to a data descriptor. If anything I would argue that we should make them look even more like data descriptors, changing |getterValue| to |value|, in order to make clients that don't care about |getterPrototypeLevel| treat them the same like regular properties.

(In reply to Mihai Sucan [:msucan] from comment #28) > (In reply to Panos Astithas [:past] from comment #26) > > The proto chain cache did not cache getter values, only the links between > > prototypes and their instances, so that a getter could be called on the > > right instance when requested. The way we now cache values means that the > > object could change state, without it being reflected in the value the > > server reports back to the client. Luckily Debugger.Object instances are > > pause-scoped in the debugger, so each time the value of a WebIDL attribute > > changes, the debuggee has been running and the next pause would generate a > > fresh Debugger.Object instance for the client to see. The web console code > > though uses long-lived Debugger.Object instances IIRC, so this could be an > > issue there. So does Firebug IIANM. Mihai and Honza would know best. > > That is correct, however this patch doesn't add getter value caching. We > only cache the known "safe getters" for every Debugger.Object. Thanks, now that I've looked at the code more carefully I see that there is no need to worry. > > I've noticed that the updated protocol spec mentions getterValue in one > > place and valueViaSafeGetter in another. I assume the former is a typo? > > I assumed the latter is a typo. Given an object safeGetterValues: { foo: { > getterValue: "bar", getterPrototypeLevel: 3 } }, I believe it makes sense to > use "getterValue" instead of the more-verbose "valueViaSafeGetter". Plus, > the protocol draft that Jim posted (on github) used "getterValue" where safe > getter descriptors were defined. I'm fine either way, but see also my |value| suggestion from comment 30. > > There may exist a remote possibility that such native getters emulating > > WebIDL attribute behavior may be provided by plugins, but even so, calling > > them after what they emulate seems appropriate to me. Note that our current > > heuristic does not work for all kinds of native getters, like the ones that > > were present before the Paris-bindings effort (e.g. bug 560072). > > > > On that matter, the try/catch around getPropertyDescriptor now silently > > ignores NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, whereas it used to create a dummy > > descriptor with the error value before, as a means to educate about the > > problem (see bug 830818 comment 23). If that was a conscious decision then > > that's fine, just wanted to call that out. > > Any such property can now be found on its prototype, unaltered. Should we > present it in safeGetterValues with getterValue showing the error value? I was actually thinking about the more generic bug 560072, but I now see that it has been fixed, so I'm no longer worried about it. > > And finally one weird thing I observed was in the > > http://htmlpad.org./debugger/ test page. Clicking the 'Click me' button and > > inspecting the pAsProto variable in the debugger shows onmouseenter and > > onmouseleave properties on the instace but with undefined values. This seems > > wrong, since such event listeners on DOM elements have the value null when > > not set, which is indeed the case for pAsProto.__proto__. The same thing > > happens for the docAsProto variable as well. > > I also noticed that. it seems to happen because those properties are "safe > getters" that can apply to |pAsProto|, not only to |p|. > > Should we open a follow-up bug to clean things up? Maybe even get it in > aurora. I'll file a followup to investigate the above, which strikes me as incorrect behavior and another one to add the configurable property, unless Jim disagrees. I don't think we need to worry about uplifting this patch, because the web console is unaffected and for the debugger it's a bug, but not a regression. Feel free to request for approval if you disagree though.

(In reply to Mihai Sucan [:msucan] from comment #28) > > > Displaying "native-getter" in the property tooltip seems rather confusing, > > both because it's not a standard JS descriptor property and because web devs > > may not be familiar with how WebIDL attributes are implemented in browser > > engines (in "native" code, usually C++). Perhaps the former issue is not so > > bad, in which case we should probably add sealed/frozen/non-extensible there > > as well, in bug 760370. The latter may be remedied by renaming native-getter > > to WebIDL-attr or somesuch. > > Ah, good point. "WebIDL-attr" is probably better. I didn't consider the > tooltip a strict list of JS property descriptors, that's why I added that > there - otherwise how would we inform the user that's some kind of different > property? > Was there a bug filed about this? Also, is it *really* necessary to mark such properties as WebIDL attributes? Bug 871985 adds even more stuff to the tooltip, and I'm a bit worried about the noise.

(In reply to Victor Porof [:vp] from comment #32) Err, bug 760370, not 871985.

(In reply to Victor Porof [:vp] from comment #32) > Was there a bug filed about this? Also, is it *really* necessary to mark > such properties as WebIDL attributes? Bug 871985 adds even more stuff to the > tooltip, and I'm a bit worried about the noise. Not yet (I neglected it), but we need to distinguish them somehow, don't we? They are not actual properties, but perhaps the web developer doesn't care about the distinction. I'd like to poll the team for opinions.

(In reply to Panos Astithas [:past] from comment #34) > (In reply to Victor Porof [:vp] from comment #32) > > Was there a bug filed about this? Also, is it *really* necessary to mark > > such properties as WebIDL attributes? Bug 871985 adds even more stuff to the > > tooltip, and I'm a bit worried about the noise. > > Not yet (I neglected it), but we need to distinguish them somehow, don't we? > They are not actual properties, but perhaps the web developer doesn't care > about the distinction. I'd like to poll the team for opinions. I think being able to tell the difference between an own-property and a webidl attribute is important enough to show as a piece of information in the tooltip.

I confirm the fix is verified on Mac OS 10.9 using FF23b5 Build ID: 20130711122148