| Summary: | Web Inspector: console.log fires getters for deep properties | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brandon Dail <kierkegaurd> | ||||
| Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bburg, commit-queue, ews-watchlist, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | Safari 11 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
Interesting that this happens when logging `outer` but not `inner`. Seems then it might be an accident in the property preview path. Switching to `console.log` to `console.trace` gives us:
[Log] Trace: getter fired Console Evaluation (Console Evaluation 1:6)
_isPreviewableObjectInternal
_appendPropertyPreviews
_generatePreview
RemoteObject
create
log
Console Evaluation (Console Evaluation 1:13)
evaluateWithScopeExtension
_evaluateOn
_evaluateAndWrap
It looks like at the bottom of _isPreviewableObjectInternal we access properties `object[propertyName]` without regard to whether or not it may be a getter.
_isPreviewableObjectInternal(object, knownObjects, depth)
{
...
// Objects are simple if they have 3 or less simple properties.
let ownPropertyNames = Object.getOwnPropertyNames(object);
if (ownPropertyNames.length > 3)
return false;
for (let i = 0; i < ownPropertyNames.length; ++i) {
let propertyName = ownPropertyNames[i];
if (!this._isPreviewableObjectInternal(object[propertyName], knownObjects, depth))
return false;
}
return true;
}
Created attachment 346936 [details]
[PATCH] Proposed Fix
Comment on attachment 346936 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=346936&action=review > Source/JavaScriptCore/inspector/InjectedScriptSource.js:1295 > + let descriptor = Object.getOwnPropertyDescriptor(object, propertyName); Might be worth verifying that we're ok w/ this doing effects? Like I believe this calls a trap in a proxy. (In reply to Saam Barati from comment #5) > Comment on attachment 346936 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346936&action=review > > > Source/JavaScriptCore/inspector/InjectedScriptSource.js:1295 > > + let descriptor = Object.getOwnPropertyDescriptor(object, propertyName); > > Might be worth verifying that we're ok w/ this doing effects? Like I believe > this calls a trap in a proxy. Is there a way to tell if its a getter without an observable effect (without being a built-in)? Comment on attachment 346936 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=346936&action=review r=me >>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1295 >>> + let descriptor = Object.getOwnPropertyDescriptor(object, propertyName); >> >> Might be worth verifying that we're ok w/ this doing effects? Like I believe this calls a trap in a proxy. > > Is there a way to tell if its a getter without an observable effect (without being a built-in)? Nope, not in a general way. Comment on attachment 346936 [details] [PATCH] Proposed Fix Clearing flags on attachment: 346936 Committed r234780: <https://trac.webkit.org/changeset/234780> All reviewed patches have been landed. Closing bug. |
With the following code the "ref" getter will be called. This is inconsistent with Firefox and Chrome, which do not fire the getter. ``` let inner = {}; Object.defineProperty(inner, "ref", { get: () => console.log("getter fired"), }); let outer = { inner }; console.log(outer); ``` This came up in the React repo: https://github.com/facebook/react/issues/13179