Bug 187542 - Web Inspector: console.log fires getters for deep properties
Summary: Web Inspector: console.log fires getters for deep properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: Safari 11
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-10 17:33 PDT by Brandon Dail
Modified: 2018-08-13 10:46 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (4.52 KB, patch)
2018-08-10 15:45 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brandon Dail 2018-07-10 17:33:27 PDT
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
Comment 1 Radar WebKit Bug Importer 2018-08-02 14:25:38 PDT
<rdar://problem/42873158>
Comment 2 Joseph Pecoraro 2018-08-02 16:18:48 PDT
Interesting that this happens when logging `outer` but not `inner`. Seems then it might be an accident in the property preview path.
Comment 3 Joseph Pecoraro 2018-08-10 15:07:27 PDT
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;
    }
Comment 4 Joseph Pecoraro 2018-08-10 15:45:38 PDT
Created attachment 346936 [details]
[PATCH] Proposed Fix
Comment 5 Saam Barati 2018-08-10 17:06:39 PDT
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.
Comment 6 Joseph Pecoraro 2018-08-10 19:04:24 PDT
(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 7 Saam Barati 2018-08-10 19:15:30 PDT
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 8 WebKit Commit Bot 2018-08-10 19:43:54 PDT
Comment on attachment 346936 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 346936

Committed r234780: <https://trac.webkit.org/changeset/234780>
Comment 9 WebKit Commit Bot 2018-08-10 19:43:55 PDT
All reviewed patches have been landed.  Closing bug.