WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
137306
Web Inspector: Expanding event objects in console shows undefined for most values, it should have real values
https://bugs.webkit.org/show_bug.cgi?id=137306
Summary
Web Inspector: Expanding event objects in console shows undefined for most va...
Joseph Pecoraro
Reported
2014-10-01 12:30:21 PDT
* 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.
Attachments
[PATCH] Proposed Fix
(32.86 KB, patch)
2015-01-16 20:16 PST
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
[IMAGE] MouseEvent - with patch
(194.64 KB, image/png)
2015-01-16 20:16 PST
,
Joseph Pecoraro
no flags
Details
[IMAGE] MouseEvent - with patch - iOS 8
(191.00 KB, image/png)
2015-01-16 20:17 PST
,
Joseph Pecoraro
no flags
Details
[IMAGE] MouseEvent - without patch
(128.40 KB, image/png)
2015-01-16 20:18 PST
,
Joseph Pecoraro
no flags
Details
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2014-10-01 12:31:03 PDT
<
rdar://problem/18517505
>
Joseph Pecoraro
Comment 2
2014-12-15 16:47:54 PST
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
Comment 3
2014-12-15 16:53:46 PST
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
Comment 4
2014-12-16 11:16:20 PST
***
Bug 139655
has been marked as a duplicate of this bug. ***
Joseph Pecoraro
Comment 5
2015-01-16 14:01:22 PST
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
Comment 6
2015-01-16 20:16:01 PST
Created
attachment 244833
[details]
[PATCH] Proposed Fix
Joseph Pecoraro
Comment 7
2015-01-16 20:16:49 PST
Created
attachment 244834
[details]
[IMAGE] MouseEvent - with patch
Joseph Pecoraro
Comment 8
2015-01-16 20:17:25 PST
Created
attachment 244835
[details]
[IMAGE] MouseEvent - with patch - iOS 8
Joseph Pecoraro
Comment 9
2015-01-16 20:18:47 PST
Created
attachment 244836
[details]
[IMAGE] MouseEvent - without patch
Timothy Hatcher
Comment 10
2015-01-16 20:41:06 PST
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
Comment 11
2015-01-19 14:09:41 PST
(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
Comment 12
2015-01-19 14:13:46 PST
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
Comment 13
2015-01-20 15:08:34 PST
Still reviewable.
WebKit Commit Bot
Comment 14
2015-01-20 16:34:55 PST
Comment on
attachment 244833
[details]
[PATCH] Proposed Fix Clearing flags on attachment: 244833 Committed
r178768
: <
http://trac.webkit.org/changeset/178768
>
WebKit Commit Bot
Comment 15
2015-01-20 16:35:00 PST
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 16
2015-01-20 21:34:28 PST
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
Comment 17
2015-01-20 21:46:48 PST
(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
Comment 18
2015-01-20 22:36:39 PST
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
Comment 19
2015-01-21 15:48:55 PST
I have a fix for the ASSERT.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug