Bug 137306 – Web Inspector: Expanding event objects in console shows undefined for most values, it should have real values

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* TEST
    <script>
    document.addEventListener("click", function(e) {
        console.log(e); x = e;
    }, false);
    </script>

* STEPS TO REPRODUCE
1. Inspect test
2. Click the document
3. In console expand MouseEvent, and the __proto__
  => all values are undefined, expected numeric values for obvious things like clientX and clientY

* REGRESSION
Yes. And this seems pretty annoying.

[Radar WebKit Bug Importer]

<rdar://problem/18517505>

[Joseph Pecoraro]

Seems like under the hood the inspector depends on the Object.getOwnPropertyNames.

Given a simple test:

    <button id="x">Click Me</button>
    <script>
    document.getElementById('x').addEventListener('click', function(event) {
        console.log( Object.getOwnPropertyNames(event) );
    }, false);
    </script>

Browsers seem to disagree:

    Safari: ["clipboardData"]
    Firefox: [ "isTrusted" ]
    Chrome: ["dataTransfer", "which", "toElement", "fromElement", "y", "x", "offsetY", "offsetX", "webkitMovementY", "webkitMovementX", "movementY", "movementX", "relatedTarget", "button", "metaKey", "altKey", "shiftKey", "ctrlKey", "clientY", "clientX", "screenY", "screenX", "keyCode", "charCode", "pageY", "pageX", "layerY", "layerX", "detail", "view", "clipboardData", "path", "cancelBubble", "returnValue", "srcElement", "defaultPrevented", "timeStamp", "cancelable", "bubbles", "eventPhase", "currentTarget", "target", "type"]


So, it looks like what is happening is:

  1. Inspector builds RemoteObject for “event”
    => gets a shallow list of event’s own properties and __proto__
    => the own properties list is pretty shallow, most event properties are on the __proto__
    => inspector UI builds an object tree (ObjectPropertiesSection)
  2. When expanding __proto__ Inspector builds a RemoteObject for “event.__proto__”
    => gets a shallow list of event.__proto__’s pwn properties, using event.__proto__ as the base obj
    => the list of properties is correct
    => the values of properties is incorrect (event.__proto__[x] !== event[x])

So, this looks totally broken.

My interpretation of this is that prototype properties would have never worked in an ObjectPropertiesSection, as they would always have been relative to the wrong object.

Doing the same steps manually in the console I would have gotten a deprecation warning anyways:

    js> Object.getOwnPropertyDescriptor(x.__proto__.screenX)
    [Error] Deprecated attempt to access property 'screenX' on a non-MouseEvent object.
    < undefined

The move of properties to prototype is expected behavior defined by WebIDL implemented in WebKit in:
http://trac.webkit.org/changeset/r163562

[Joseph Pecoraro]

I think we need to make a couple changes here.

Given: js> console.log(o)

  (1) First, get all the properties on "o" (all prototypes, all configurations)
     => expanding "o" you will see ALL the properties and values, no matter where they are in the prototype chain
     => we can display own properties / non-own properties differently if we feel that is useful.
     - requires a change in InjectedScriptSource.js

  (2) Expanding prototypes, get all the own properties of that prototype and show their value on "o"
     => now you will see specifically the properties defined by that prototype
     => they will have real values, instead of always undefined and a silenced exception.

[Joseph Pecoraro]

*** Bug 139655 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Currently ObjectPropertiesSection populates either ALL properties (ScopeChainSidebar only) or OWN properties (normal case). ObjectPropertyTreeElement populates just OWN properties.

Instead, what I want to do is make the normal case fetch not just OWN properties, but OWN+GETTER properties. The only time we should fetch OWN properties is describing a __proto__.

What OWN+GETTER properties means, is OWN properties, and all the getter properties from parent prototypes.

--

That should meant that logging an Event object, you will immediately get all of the useful properties and values on that object. When you expand the __proto__ chain, it will be to see the APIs of those prototypes (what getters/setters they define, any own property constants on those prototypes, any methods they define).

--

There will be next steps after this:

(1) WebKit's Native Property descriptors are invalid. Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey") gives us a descriptor with "get" undefined. It should be a function.

(2) We probably don't want to always evaluate a getter. For the native MouseEvent.prototype.altKey getter that should be fine, but for a non-native getter Foo.prototype.foo, we may not want to. We should detect this case, and provide a way to manually evaluate the getter or not.

(3) In the case of overridden getters, users might want to get the value of the getter in a parent prototype. If both SuperClassA and SuperClassB define a getter for "foo", you might want to know the SuperClassA and SuperClassB values of "foo". With the above proposal, __proto__'s will just show APIs, to then interact with the object will require another step.

[Joseph Pecoraro]

Created attachment 244833 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 244834 [details]
[IMAGE] MouseEvent - with patch

[Joseph Pecoraro]

Created attachment 244835 [details]
[IMAGE] MouseEvent - with patch - iOS 8

[Joseph Pecoraro]

Created attachment 244836 [details]
[IMAGE] MouseEvent - without patch

[Timothy Hatcher]

I wouldn't expect to see the getters still under the prototype. We should remove them from there when they are promoted to the object. Agree?

[Joseph Pecoraro]

(In reply to comment #10)
> I wouldn't expect to see the getters still under the prototype. We should
> remove them from there when they are promoted to the object. Agree?

Ideally no-one will need to expand the __proto__ chain anymore. If they do, they are looking at the prototype chain API surface (constants, functions, getter/setter functions). In this case I think it is fine to show the getters/setters under the prototype. We should improve their appearance in some way. Currently they are undefined because of bug 140575, but they should be functions. However we make them appear, I think showing them, alongside the other function properties, is useful.

[Joseph Pecoraro]

Another way of thinking about this is to make the own properties visually distinct at the first level. If you have:

  function Foo() { this._foo = 5; }
  Foo.prototype = {
    __proto__: Object.prototype,
    get foo() { return this._foo; }
  };

Then showing the object:

  console.log(new Foo);

We will see:

  _foo // OWN property on the object itself
  foo  // getter from prototype chain (Foo.prototype.foo), not an OWN property, expect to see a "get foo" somewhere if you expand the prototype chain

This way if you have lots of properties that are actually getters in the prototype chain appearing at the top level they could be distinct from the real own properties that won't have anything higher up in the prototype chain.

[Joseph Pecoraro]

Still reviewable.

[WebKit Commit Bot]

Comment on attachment 244833 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 244833

Committed r178768: <http://trac.webkit.org/changeset/178768>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Joe landed an update to inspector/model/remote-object-get-properties.html in <http://trac.webkit.org/changeset/178795>.

However, the test still crashes and times out on various bots, see <https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object-get-properties.html>.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010c5fed7a WTFCrash + 42 (Assertions.cpp:321)
1   com.apple.JavaScriptCore      	0x000000010c223ee9 Inspector::Protocol::PrimitiveBindingTraits<(Inspector::InspectorValue::Type)1>::assertValueHasExpectedType(Inspector::InspectorValue*) + 89 (InspectorProtocolTypes.h:125)
2   com.apple.JavaScriptCore      	0x000000010c222af1 Inspector::Protocol::BindingTraits<Inspector::Protocol::Runtime::PropertyDescriptor>::assertValueHasExpectedType(Inspector::InspectorValue*) + 1297 (InspectorProtocolObjects.cpp:615)
3   com.apple.JavaScriptCore      	0x000000010c19d31c Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector::Protocol::Runtime::PropertyDescriptor> >::assertValueHasExpectedType(Inspector::InspectorValue*) + 236 (InspectorProtocolTypes.h:153)
4   com.apple.JavaScriptCore      	0x000000010c19c26c Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector::Protocol::Runtime::PropertyDescriptor> >::runtimeCast(WTF::RefPtr<Inspector::InspectorValue>&&) + 204 (InspectorProtocolTypes.h:139)
...

I think that I'm going to roll out for now.

[Joseph Pecoraro]

(In reply to comment #16)
> Joe landed an update to inspector/model/remote-object-get-properties.html in
> <http://trac.webkit.org/changeset/178795>.
> 
> However, the test still crashes and times out on various bots, see
> <https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object-get-properties.
> html>.
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x000000010c5fed7a WTFCrash + 42
> (Assertions.cpp:321)
> 1   com.apple.JavaScriptCore      	0x000000010c223ee9
> Inspector::Protocol::PrimitiveBindingTraits<(Inspector::InspectorValue::
> Type)1>::assertValueHasExpectedType(Inspector::InspectorValue*) + 89
> (InspectorProtocolTypes.h:125)
> 2   com.apple.JavaScriptCore      	0x000000010c222af1
> Inspector::Protocol::BindingTraits<Inspector::Protocol::Runtime::
> PropertyDescriptor>::assertValueHasExpectedType(Inspector::InspectorValue*)
> + 1297 (InspectorProtocolObjects.cpp:615)
> 3   com.apple.JavaScriptCore      	0x000000010c19d31c
> Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector::
> Protocol::Runtime::PropertyDescriptor>
> >::assertValueHasExpectedType(Inspector::InspectorValue*) + 236
> (InspectorProtocolTypes.h:153)
> 4   com.apple.JavaScriptCore      	0x000000010c19c26c
> Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector::
> Protocol::Runtime::PropertyDescriptor>
> >::runtimeCast(WTF::RefPtr<Inspector::InspectorValue>&&) + 204
> (InspectorProtocolTypes.h:139)
> ...
> 
> I think that I'm going to roll out for now.

Wow, I didn't notice that. Please roll out, I'll address this tomorrow.

[Alexey Proskuryakov]

There are two other changes blocking rollout (http://trac.webkit.org/changeset/178791 and http://trac.webkit.org/projects/webkit/changeset/178796), so it seems more practical to skip the test until tomorrow. I'll do that.

[Joseph Pecoraro]

I have a fix for the ASSERT.