Bug 176766 - Web Inspector: Implement `queryObjects` Command Line API
Summary: Web Inspector: Implement `queryObjects` Command Line API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 200520
  Show dependency treegraph
 
Reported: 2017-09-11 21:28 PDT by Joseph Pecoraro
Modified: 2019-08-07 19:38 PDT (History)
17 users (show)

See Also:


Attachments
[Patch] WIP - missing tests (13.37 KB, patch)
2017-09-16 23:21 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.37 MB, application/zip)
2017-09-17 00:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.52 MB, application/zip)
2017-09-17 00:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.23 MB, application/zip)
2017-09-17 00:54 PDT, Build Bot
no flags Details
Patch (20.34 KB, patch)
2017-09-18 23:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (2.20 MB, application/zip)
2017-09-19 00:41 PDT, Build Bot
no flags Details
Patch (20.49 KB, patch)
2017-09-19 11:33 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (22.99 KB, patch)
2017-09-20 14:29 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (35.82 KB, patch)
2017-09-21 22:48 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (36.55 KB, patch)
2017-09-22 23:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (950.38 KB, application/zip)
2017-09-23 01:24 PDT, Build Bot
no flags Details
Patch (34.10 KB, patch)
2017-09-23 02:09 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (37.63 KB, patch)
2017-09-27 15:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (18.64 KB, patch)
2018-11-30 14:38 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (38.18 KB, patch)
2018-11-30 20:34 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (41.38 KB, patch)
2018-11-30 20:58 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-sierra (2.07 MB, application/zip)
2018-11-30 23:00 PST, Build Bot
no flags Details
Patch (41.47 KB, patch)
2018-12-01 00:45 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (44.04 KB, patch)
2019-01-02 17:55 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-09-11 21:28:29 PDT
Chrome DevTools just introduced a `queryObjects` Command Line API:
https://developers.google.com/web/updates/2017/08/devtools-release-notes#query-objects

> Call queryObjects(Constructor) from the Console to return an array of objects
> that were created with the specified constructor. For example:
> 
>   * queryObjects(Promise). Returns all Promises.
>   * queryObjects(HTMLElement). Returns all HTML elements.
>   * queryObjects(foo), where foo is a function name. Returns all objects that were instantiated via new foo().

Example in action:
https://twitter.com/ak_239/status/902927604507844608

This would be very useful for developers. I often find myself in the situation where I want to find all the objects of a particular class, knowing that there will be (or should be) just a few instances.
Comment 1 Joseph Pecoraro 2017-09-11 21:41:40 PDT
I'd expect something like: (untested)

    Vector<JSCell> queryObjects(ExecState* exec, JSValue *proto)
    {
        Vector<JSCell> instances;

        HeapIterationScope iterationScope(m_vm.heap);
        m_vm.heap.objectSpace().forEachLiveCell([&instances] (HeapCell* cell, HeapCell::Kind kind) {
            if (kind != HeapCell::JSCell)
                return IterationStatus::Continue;

            JSCell* value = static_cast<JSCell*>(heapCell);
            if (JSObject::defaultHasInstance(exec, value, proto))
                instances.append(cell);

            return IterationStatus::Continue;
        });

        return instances;
    }

We probably would want to avoid exposing private / built-in instances of objects. So we would need to special case or deny "Function", which might expose `Promise` and `@Promise`.

"Object" would be at risk of exposing plain Objects inside of builtins, like:

    var promiseCapability = {
        @promise: @undefined,
        @resolve: @undefined,
        @reject: @undefined
    };

but I don't think that is actually a problem because user script can't modify the private @properties of the object itself.
Comment 2 Joseph Pecoraro 2017-09-11 21:52:46 PDT
Note to self, for us this would make the most sense on `InjectedScriptHost` instead of `CommandLineAPIHost` so that it can be implemented in `BasicCommandLineAPI` (see InjectedScriptSource.js) and therefore work in JSContext inspection.
Comment 3 Saam Barati 2017-09-12 01:01:46 PDT
I think the tricky thing here is not exposing internal objects. Internal fields are safe since a user script has no way to access them. I’m more worried about the one+ level(s) of indirection in some builtins like so:

let internalObject = {userExposedField: ...}
let userExposedObject {@internalField: internalObject}

If you query for Object, you’ll see internalObject, and it will have fields which are not private. This is ok in the context of normal JS since the only way to get to that object is via @internalField. However, when iterating over the entire heap, we have no obvious way of determining this.
Comment 4 Sam Weinig 2017-09-12 14:33:23 PDT
Should it be in https://console.spec.whatwg.org?
Comment 5 Joseph Pecoraro 2017-09-12 15:13:51 PDT
(In reply to Sam Weinig from comment #4)
> Should it be in https://console.spec.whatwg.org?

The Console object is different from the Command Line API.

• The Console standard specifies properties exposed on `console`. Things like `console.log`, `console.trace` etc. The link you provide is the best standardization effort for that. A good reference is:
https://developers.google.com/web/tools/chrome-devtools/console/console-reference

• The Command Line API (no standard effort that I know of) is a list of helpful functions available in the console / REPL of developer tools. For example `$0`, `$$`, `copy`, etc. Perhaps the best reference right now is:
https://developers.google.com/web/tools/chrome-devtools/console/command-line-reference

And the older reference that most people know of:
https://getfirebug.com/wiki/index.php/Command_Line_API
Comment 6 Sam Weinig 2017-09-12 17:27:48 PDT
(In reply to Joseph Pecoraro from comment #5)
> (In reply to Sam Weinig from comment #4)
> > Should it be in https://console.spec.whatwg.org?
> 
> The Console object is different from the Command Line API.
> 
> • The Console standard specifies properties exposed on `console`. Things
> like `console.log`, `console.trace` etc. The link you provide is the best
> standardization effort for that. A good reference is:
> https://developers.google.com/web/tools/chrome-devtools/console/console-
> reference
> 
> • The Command Line API (no standard effort that I know of) is a list of
> helpful functions available in the console / REPL of developer tools. For
> example `$0`, `$$`, `copy`, etc. Perhaps the best reference right now is:
> https://developers.google.com/web/tools/chrome-devtools/console/command-line-
> reference
> 
> And the older reference that most people know of:
> https://getfirebug.com/wiki/index.php/Command_Line_API

My bad.
Comment 7 Devin Rousso 2017-09-16 23:21:38 PDT
Created attachment 321026 [details]
[Patch] WIP - missing tests
Comment 8 Build Bot 2017-09-17 00:29:05 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-09-17 00:29:07 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-09-17 00:33:50 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-09-17 00:33:52 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-09-17 00:54:44 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-09-17 00:54:46 PDT Comment hidden (obsolete)
Comment 14 Joseph Pecoraro 2017-09-18 12:10:42 PDT
Comment on attachment 321026 [details]
[Patch] WIP - missing tests

View in context: https://bugs.webkit.org/attachment.cgi?id=321026&action=review

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:632
> +    JSValue prototype;

Unused.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:640
> +    PropertySlot slot(object->getPrototypeDirect(), PropertySlot::InternalMethodType::VMInquiry);
> +    PropertyName prototypeProperty(vm.propertyNames->prototype);
> +    if (object->getPropertySlot(exec, prototypeProperty, slot)) {
> +        EXCEPTION_ASSERT(!scope.exception());
> +        if (slot.isValue())
> +            prototypeOrConstructor = slot.getValue(exec, prototypeProperty);
> +    }

I'm not sure I understand why the slot is constructed with `object->getPrototypeDirect()` and the code below just uses `object->getPropertySlot`. I see that `JSObject::calculatedClassName` does this as well.

I also don't think we need to be as careful as VMInquiry. I think we can just do a get of the "prototype" property. So I'd expect something more like:

    JSValue value = exec->argument(0);
    if (!value.isObject())
        return throwTypeError(...);

    JSObject* object = asObject(value);
    JSValue prototype = object;

    JSValue constructorPrototype = object->get(exec, vm.propertyNames->prototype);
    RETURN_IF_EXCEPTION(scope, { });
    if (constructorPrototype.isObject())
        prototype = asObject(constructorPrototype); 

    ... using "prototype" ...

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:642
> +    JSLockHolder lock(vm);

We shouldn't need to take the lock here. We should already be holding it because we are evaluating inside of JavaScript.
Comment 15 Devin Rousso 2017-09-18 23:20:41 PDT
Created attachment 321185 [details]
Patch

Added a few basic tests.  Still not entirely sure what to do about the case where someone calls `queryObject(Object)` or `queryObject(Array)`.  Part of me thinks it would be good to entirely block those so as to not expose any internal values, but at the same time it might be useful to see all the "nameless" objects that a program creates (or just count how many there are).
Comment 16 Build Bot 2017-09-19 00:41:17 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-09-19 00:41:18 PDT Comment hidden (obsolete)
Comment 18 Devin Rousso 2017-09-19 11:33:54 PDT
Created attachment 321223 [details]
Patch
Comment 19 Devin Rousso 2017-09-20 14:29:11 PDT
Created attachment 321372 [details]
Patch
Comment 20 Joseph Pecoraro 2017-09-20 19:26:50 PDT
Comment on attachment 321372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321372&action=review

r- just because I want to see a few more tests

> Source/JavaScriptCore/ChangeLog:12
> +        Iterates over all live JSCell in the heap to find these objects.

I would say "Does a GC and then iterates over..."

This is introducing new API. I think we should have a paragraph in the ChangeLog, above the files, that describes what we are adding. And overview of `queryObjects()` and explains our current Object/Array/Function exception and reasoning. ChangeLogs are a great opportunity to include this information, and normally it is just copying it from the Bugzilla bug description and putting it in here with polish!

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:660
> +    HeapIterationScope iterationScope(vm.heap);

I'm tempted to say you should put this in an anonymous block:

    {
        HeapIterationScope iterationScope(vm.heap);
        ...
    }

I think we want to keep the iteration scope as small as possible. In this case it wouldn't change things, but in the future if someone were to add code around this it would be less error prone to holding the scope longer than necessary.

> LayoutTests/inspector/console/queryObjects.html:30
> +        {prototypeOrConstructor: "HTMLImageElement", resultCount: 0},

Lets have a test for HTMLBodyElement / HTMLHeadElement where we expect there to be 1. Or create a few <meta> tags above and access them. We could also test that `queryObjects(Node).length > 20` or something like that.

Node is an interesting case. We do not create Node JavaScript value wrappers for all nodes unless they are needed. So queryObjects(Node) only gets the ones created, not all nodes. This actually matches Chrome's behavior:

    js> queryObjects(Node)
    Array (153)

    js> document.querySelectorAll("*")
    Array (348)
    // Expand to see all instances

    js> queryObjects(Node)
    Array (378)

So I think we are fine there. Its an interesting, but not otherwise harmful exposure of implementation behavior. A developer might get confused that queryObjects(Node).filter((x) => ...) doesn't find their target Node, but I think thats a good thing. This lets developers see the actual live instances of a type. Also, I don't think Developers will want to use this (in its intended use case) for Node. They would more likely use this for their own object / class types.

This does raise above that HTMLMetaElement might not work without first doing a `document.querySelectorAll("meta")` or something to create wrappers.

> LayoutTests/inspector/console/queryObjects.html:39
> +    ];

We should have tests for calling it with non-Object values.

Things like: 1, true, null, NaN, Symbol(), "test", String("test"), Number(1), to make sure it returns no results or throws.

Behavior in Chrome is to throw if it is not an object:

    > queryObjects(true)
    Prototype should be an Object.

    > queryObjects("test")
    Prototype should be an Object.

    > queryObjects(Symbol())
    Prototype should be instance of Object

> LayoutTests/inspector/console/queryObjects.html:76
> +            queryObjects(prototypeOrConstructor, (remoteObject, wasThrown, savedResultIndex) => {

In Chrome, no parameter produces no result without an exception.

    js> queryObjects()
    undefined

This almost makes me think we should have queryObjects take a `...list` of prototypes / constructors to enable `queryObjects(Foo, Bar)`, however right now that is not possible without users doing their own concat of each list.

I think we start by striving to match behavior and maybe reach out to the Chrome folks to more strongly specify behavior.
Comment 21 Saam Barati 2017-09-21 17:07:33 PDT
Do we have a plan about what to do about exposing internal objects? Or are we ok with just exposing them? We could mandate all builtin objects use private names, even if they're only reachable via a private name.
Comment 22 Joseph Pecoraro 2017-09-21 17:31:38 PDT
(In reply to Saam Barati from comment #21)
> Do we have a plan about what to do about exposing internal objects? Or are
> we ok with just exposing them? We could mandate all builtin objects use
> private names, even if they're only reachable via a private name.

The plan right now is to disallow Object / Array / Function. Reasons:

  • Object
    - built-ins create Objects with `x = { .. }`.
    - Most use @keys some don't.

  • Array
    - Example:
    PromiseOperations.js
    222:    this.@promiseReactions = [];

  • Function
    - Some builtin functions expect to be called "properly" with correct parameter count, types, etc.

However we both think that it would be nice to do things like:

   // Find all objects with a "payload" property.
   queryObjects(Object).filter((o) => "payload" in o);

   // Find all arrays containing `myValue`
   queryObjects(Array).filter((arr) => arr.includes(myValue));

   // Find all functions whose name ends in "AfterDelay"
   queryObjects(Function).filter((f) => f.endsWith("AfterDelay"));

We don't yet have a story for how we would avoid internal Object / Array / Functions.
Comment 23 Devin Rousso 2017-09-21 22:48:39 PDT
Created attachment 321519 [details]
Patch
Comment 24 Joseph Pecoraro 2017-09-22 11:14:43 PDT
Comment on attachment 321519 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321519&action=review

Test tests are much better. I'm fine with the patch but we should see what the JavaScriptCore folks think, to see if they think we may be leaking any internals.

> LayoutTests/inspector/console/queryObjects.html:24
> +var Stack = {
> +    Object: Object(),
> +    Function: Function(),
> +    Array: Array(),
> +    Boolean: Boolean(),
> +    String: String(),
> +    Number: Number(),
> +    Date: Date(),
> +    RegExp: RegExp(),
> +    Symbol: Symbol(),
> +};

I don't think this does what you think it does:

    (1) I believe all objects live in the heap.
        - Some objects may be optimized out in some cases ("sinking" optimizations for example)
    (2) Some of these don't create objects.
        - `Date()` for example produces a string, not a Date instance
        https://tc39.github.io/ecma262/#sec-date-constructor-date
        - `Number()` for example produces a number, not an object
        https://tc39.github.io/ecma262/#sec-number-constructor-number-value
    (3) Some of these are no different from their `new` versions
        - `Array()` allocates a new array just like `new Array`
        https://tc39.github.io/ecma262/#sec-array-constructor-array

    NOTE: The spec checks `new.target` for each. It is `undefined` if `new` was not used.

An object may only have references from the stack:

    (function() {
        // "Stack reference" within this function.
        let local = new Date;
        ...
    })();

But the instance lives in JavaScriptCore's Heap, and will be collected when there are no more references to it.

I don't understand what you are trying to test with `Stack`, and it seems incorrect. Lets drop it.

> LayoutTests/inspector/console/queryObjects.html:26
> +var Heap = {

How about just calling this: `TestObjects` or `Instances`. I don't see why it is named `Heap`.

> LayoutTests/inspector/console/queryObjects.html:123
> +        {prototypeOrConstructor: `Stack.Date`, shouldThrow: true},
> +        {prototypeOrConstructor: `Heap.Date`, resultCount: 0},
> +        {prototypeOrConstructor: `Date`},
> +        {prototypeOrConstructor: `Date.prototype`},
> +
> +        {prototypeOrConstructor: `Stack.RegExp`, resultCount: 0},
> +        {prototypeOrConstructor: `Heap.RegExp`, resultCount: 0},
> +        {prototypeOrConstructor: `RegExp`},
> +        {prototypeOrConstructor: `RegExp.prototype`},
> +
> +        {prototypeOrConstructor: `Heap.Map`, resultCount: 0},
> +        {prototypeOrConstructor: `Map`},
> +        {prototypeOrConstructor: `Map.prototype`},
> +
> +        {prototypeOrConstructor: `Heap.Set`, resultCount: 0},
> +        {prototypeOrConstructor: `Set`},
> +        {prototypeOrConstructor: `Set.prototype`},
> +
> +        {prototypeOrConstructor: `Heap.Promise`, resultCount: 0},
> +        {prototypeOrConstructor: `Promise`},
> +        {prototypeOrConstructor: `Promise.prototype`},

Testing all the types has some value, but it would be far more interesting to do this with non-trivial expectations. For example, creating some instances. Or, barring that this could be simplified to just a list of builtin types, so given just `Set`, it could automatically figure out to test the Constructor, Prototype, and an Instance.

> LayoutTests/inspector/console/queryObjects.html:163
> +    suite.addTestCase({

You could add a test case that tests that queryObjects performs a full GC. For example:

    TestObjects = {
        listOfDates: [new Date, new Date, new Date];
    };

    function clearDateList() {
        TestObjects.listOfDates = [];
    }

    // ...

    suite.addTestCase({
        name: "CommandLineAPI.queryObjects.GC",
        test(resolve, reject) {
            queryObjects(`Date`, (remoteObject, wasThrown, savedResultIndex) => {
                InspectorTest.expectThat(remoteObject.size, 3, `Should be 3 Date instances.`);

                RuntimeAgent.releaseObjectGroup("test");
                InspectorTest.evaluateInPage(`clearDateList()`);
                queryObjects(`Date`, (remoteObject, wasThrown, savedResultIndex) => {
                    InspectorTest.expectThat(remoteObject.size, 0, `Should now be 0 Date instances.`);
                    resolve();
                });
            });
        }
    });
Comment 25 Joseph Pecoraro 2017-09-22 11:16:42 PDT
Comment on attachment 321519 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321519&action=review

> LayoutTests/inspector/console/queryObjects.html:168
> +            const prototypeOrConstructor = "";
> +            queryObjects(prototypeOrConstructor, (remoteObject, wasThrown, savedResultIndex) => {
> +                InspectorTest.expectThat(wasThrown, "Calling \"queryObjects\" with no parameters should throw an exception.");

Chrome didn't throw here, I think we should match that behavior.
Comment 26 Devin Rousso 2017-09-22 23:53:36 PDT
Created attachment 321618 [details]
Patch
Comment 27 Build Bot 2017-09-23 01:24:46 PDT Comment hidden (obsolete)
Comment 28 Build Bot 2017-09-23 01:24:47 PDT Comment hidden (obsolete)
Comment 29 Joseph Pecoraro 2017-09-23 01:34:40 PDT
Comment on attachment 321618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321618&action=review

> LayoutTests/inspector/console/queryObjects-expected.txt:74
> +-- Running test case: CommandLineAPI.queryObjects.undefined
> +FAIL: Calling "queryObjects" with "undefined" should throw an exception.

Lots of FAILs in the expectations. Seems unexpected.
Comment 30 Devin Rousso 2017-09-23 02:09:04 PDT
Created attachment 321623 [details]
Patch
Comment 31 Joseph Pecoraro 2017-09-25 11:37:19 PDT
Comment on attachment 321623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321623&action=review

This looks good to me. Lets get a nod from a JSC reviewer.

> Source/JavaScriptCore/ChangeLog:22
> +        them are highlighy sensitive and undefined behaviour can occur if they are modified.

Typo: "highlighy" => "highly"
Suggestion: "if they are modified" => "If they are modified, or used incorrectly."

> LayoutTests/inspector/console/queryObjects.html:119
> +        name: "CommandLineAPI.queryObjects.GC",

I know its weird, but we should still give this a description. So if someone comes by later and this fails they have some idea of what this test is doing:

    description: "Test that queryObjects performs a synchronous GC to ensure the resulting list is as accurate as possible."
Comment 32 Saam Barati 2017-09-25 14:27:00 PDT
Comment on attachment 321623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321623&action=review

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:643
> +    JSValue constructorPrototype = object->get(exec, PropertyName(vm.propertyNames->prototype));
> +    RETURN_IF_EXCEPTION(scope, { });

Let's have a test for this throwing an exception if this is the behavior that we want.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:649
> +    if (JSGlobalObject* lexicalGlobalObject = exec->lexicalGlobalObject()) {

Why the condition here? This shouldn't be null.

I guess an interesting question here is which global object should we use: lexicalGlobalObject or object->globalObject(). I sort of think the latter, but the array you create below will be of lexicalGlobalObject

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:656
> +        // FIXME: implement a way of distinguishing between internal and user-created objects.
> +        if (object == lexicalGlobalObject->objectPrototype())
> +            return throwTypeError(exec, scope, ASCIILiteral("queryObjects cannot be called with Object."));
> +        if (object == lexicalGlobalObject->functionPrototype())
> +            return throwTypeError(exec, scope, ASCIILiteral("queryObjects cannot be called with Function."));
> +        if (object == lexicalGlobalObject->arrayPrototype())
> +            return throwTypeError(exec, scope, ASCIILiteral("queryObjects cannot be called with Array."));

I'm worried about this part of the code.

I think we could make this work with not much effort for things that aren't arrays:
1. make all builtins always use private names for all fields even if it's just using a vanilla object
2. exclude builtin functions (this is an easy thing to test for)

That said, this doesn't work for arrays. I wonder if a better solution would be to do some kind of heap analysis to determine what are the objects that can be pointed to by real JS code.

Also, within this context, I think your checks are not comprehensive enough. For example:

```
class Foo { };
Array.prototype.__proto__ = Foo.prototype;
```

I think `queryObjects(Foo)` in this world will give you all arrays.

You can do the same thing for Function, but can't for Object, since Object.prototype.__proto__ is not assignable.

Also, do we know these are the only three sacred types? I thought we have some Builtins that rely on Map/Set, or maybe even other types. I might be misremembering though.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:660
> +    vm.heap.collectNow(Sync, CollectionScope::Full);

Do we really want to do a full GC on every call?

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:673
> +                array->push(exec, value);

We don't want push() here. push() is observable by JS code. We probably want something else, I think may putDirectIndex or something similar. It's might be worth writing a test for this.
Comment 33 Devin Rousso 2017-09-26 21:44:18 PDT
Comment on attachment 321623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321623&action=review

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:656
>> +            return throwTypeError(exec, scope, ASCIILiteral("queryObjects cannot be called with Array."));
> 
> I'm worried about this part of the code.
> 
> I think we could make this work with not much effort for things that aren't arrays:
> 1. make all builtins always use private names for all fields even if it's just using a vanilla object
> 2. exclude builtin functions (this is an easy thing to test for)
> 
> That said, this doesn't work for arrays. I wonder if a better solution would be to do some kind of heap analysis to determine what are the objects that can be pointed to by real JS code.
> 
> Also, within this context, I think your checks are not comprehensive enough. For example:
> 
> ```
> class Foo { };
> Array.prototype.__proto__ = Foo.prototype;
> ```
> 
> I think `queryObjects(Foo)` in this world will give you all arrays.
> 
> You can do the same thing for Function, but can't for Object, since Object.prototype.__proto__ is not assignable.
> 
> Also, do we know these are the only three sacred types? I thought we have some Builtins that rely on Map/Set, or maybe even other types. I might be misremembering though.

Wow I definitely didn't consider this!  From what I can tell, it looks like we use Set in InjectedScriptSource.js, so it might be worth adding that too.

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:673
>> +                array->push(exec, value);
> 
> We don't want push() here. push() is observable by JS code. We probably want something else, I think may putDirectIndex or something similar. It's might be worth writing a test for this.

When you say observable, does that mean like if the user overwrites Array.prototype.push to do something else, or is this a different meaning?
Comment 34 Devin Rousso 2017-09-27 15:35:22 PDT
Created attachment 322029 [details]
Patch
Comment 35 Devin Rousso 2017-10-05 14:22:08 PDT
Are there any other pieces of feedback on this?

I tried testing the observability of putDirectIndex manually (override Array.prototype.push and add a console.log) and, from what I could tell, it looked like it was no longer observable.
Comment 36 Brian Burg 2017-10-09 11:40:13 PDT
(In reply to Devin Rousso from comment #35)
> Are there any other pieces of feedback on this?
> 
> I tried testing the observability of putDirectIndex manually (override
> Array.prototype.push and add a console.log) and, from what I could tell, it
> looked like it was no longer observable.

Saam, can you review this patch again?
Comment 37 Radar WebKit Bug Importer 2017-10-09 11:40:40 PDT
<rdar://problem/34890688>
Comment 38 Radar WebKit Bug Importer 2017-10-09 11:40:40 PDT
<rdar://problem/34890689>
Comment 39 Saam Barati 2017-10-09 11:44:48 PDT
(In reply to Devin Rousso from comment #35)
> Are there any other pieces of feedback on this?

I've been lagging on reviewing this because I don't know what the right direction is. If we leak internal objects, we're creating a security exploit that just requires opening web inspector. I think we need a patch that guarantees we leak no internal objects, and I'm not sure if this patch does that. Going to read the code again.

Fil, maybe you have an idea on how to accomplish this?
 
> I tried testing the observability of putDirectIndex manually (override
> Array.prototype.push and add a console.log) and, from what I could tell, it
> looked like it was no longer observable.

I think this is correct.
Comment 40 Saam Barati 2017-10-09 11:49:42 PDT
Comment on attachment 322029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322029&action=review

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:635
> +    // Check that the prototype chain of proto hasn't been modified to include value.
> +    return JSObject::defaultHasInstance(exec, proto, value);

I think all it takes is a ProxyObject to spoof this.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:653
> +    JSValue constructorPrototype = object->get(exec, PropertyName(vm.propertyNames->prototype));

Nit: You don't need PropertyName constructor here, I believe it has an implicit constructor

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:663
> +    if (checkForbiddenPrototype(exec, object, lexicalGlobalObject->objectPrototype()))
> +        return throwTypeError(exec, scope, ASCIILiteral("queryObjects cannot be called with Object."));

How does this not include just about every single object?

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:687
> +            if (JSObject::defaultHasInstance(exec, value, prototype))
> +                array->putDirectIndex(exec, array->length(), value);

I think all it takes is a ProxyObject to spoof this
Comment 41 Devin Rousso 2017-10-09 13:21:35 PDT
Comment on attachment 322029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322029&action=review

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:635
>> +    return JSObject::defaultHasInstance(exec, proto, value);
> 
> I think all it takes is a ProxyObject to spoof this.

I might not be doing this right (I haven't worked with Proxy much), but I tried doing the following and I still got the error "queryObjects cannot be called with Object.", which is what I'd expect.

```
let proxy = new Proxy({}, {
    get(target, property, receiver) {
        return Object.prototype;
    }
});

queryObjects(proxy);
```

Were you thinking of a different usage?

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:663
>> +        return throwTypeError(exec, scope, ASCIILiteral("queryObjects cannot be called with Object."));
> 
> How does this not include just about every single object?

checkForbiddenPrototype ensures that `object` isn't equal to Object.prototype or that Object.prototype hasn't been modified to include `object` in the chain.  In the case of Object, since it's prototype is not modifiable, this is unnecessary.  It is still needed for Function, Array, Map, and Set.

This was based on your comment <https://bugs.webkit.org/show_bug.cgi?id=176766#c32>

> For example:
> 
> ```
> class Foo { };
> Array.prototype.__proto__ = Foo.prototype;
> ```
> 
> I think `queryObjects(Foo)` in this world will give you all arrays.
> 
> You can do the same thing for Function, but can't for Object, since
> Object.prototype.__proto__ is not assignable.
Comment 42 Saam Barati 2017-10-10 19:40:26 PDT
Comment on attachment 322029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322029&action=review

>>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:635
>>> +    return JSObject::defaultHasInstance(exec, proto, value);
>> 
>> I think all it takes is a ProxyObject to spoof this.
> 
> I might not be doing this right (I haven't worked with Proxy much), but I tried doing the following and I still got the error "queryObjects cannot be called with Object.", which is what I'd expect.
> 
> ```
> let proxy = new Proxy({}, {
>     get(target, property, receiver) {
>         return Object.prototype;
>     }
> });
> 
> queryObjects(proxy);
> ```
> 
> Were you thinking of a different usage?

The Proxy as you have defined it doesn't run into this issue, but you could imagine it being something like:

```
let counter = 0;
let proxy = new Proxy({}, { get(target, property, receiver) { return counter++ === 0 ? somethingToTrickInspector : Object.prototype; } });
```
Comment 43 Joseph Pecoraro 2017-10-10 19:45:04 PDT
Comment on attachment 322029 [details]
Patch

r-, we need to take an approach that doesn't have the problem of leaking built-in values, which would be quite problematic.

After talking with Saam we have some possible solutions, but none are trivial. r- on this patch for now.
Comment 44 Devin Rousso 2018-11-30 14:38:29 PST
Created attachment 356241 [details]
Patch

Given that most (if not all) of the "internal" objects are exposed by a heap snapshot, I think we should revisit this
Comment 45 Joseph Pecoraro 2018-11-30 14:50:41 PST
Comment on attachment 356241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356241&action=review

I agree. r- because the test is missing.

I think with the Object/Array restriction this is still useful and unlikely to be dangerous.

> Source/JavaScriptCore/ChangeLog:28
> +        them are highlighy sensitive and undefined behaviour can occur if they are modified.

Typo: "highlighy"

> Source/WebCore/ChangeLog:9
> +        Test: inspector/console/queryObjects.html

r-, where is the test?

> Source/WebCore/inspector/CommandLineAPIModuleSource.js:60
> +    for (let method of CommandLineAPI.methods) {

Hmm, we normally don't use `for..of` in Injected Scripts because it is observable if the page overrides the Array prototype Symboling.iterator. InjectedScriptSource is pretty keen to not use it.

> Source/WebCore/inspector/CommandLineAPIModuleSource.js:242
> +    queryObjects(...args)
> +    {
> +        return InjectedScriptHost.queryObjects(...args);
> +    },

Can we enumerate them to avoid potentially using an observable iterator here. Seems like there is only 1 supported argument anyways right now.
Comment 46 Devin Rousso 2018-11-30 20:34:32 PST
Created attachment 356288 [details]
Patch

Oops.  Looks like webkit-patch doesn't consider untracked files :/
Comment 47 Devin Rousso 2018-11-30 20:58:03 PST
Created attachment 356290 [details]
Patch
Comment 48 Build Bot 2018-11-30 23:00:28 PST Comment hidden (obsolete)
Comment 49 Build Bot 2018-11-30 23:00:30 PST Comment hidden (obsolete)
Comment 50 Devin Rousso 2018-12-01 00:45:18 PST
Created attachment 356300 [details]
Patch
Comment 51 Joseph Pecoraro 2018-12-03 18:26:56 PST
Comment on attachment 356300 [details]
Patch

r=me, Though you should get a JSC engineer (Saam?) to sign off on this as well.
Comment 52 Yusuke Suzuki 2018-12-04 14:56:01 PST
Is Promise and Promise.prototype safe?
If we have a promise which holds a sensitive value, this leaks it.
Comment 53 Yusuke Suzuki 2018-12-04 14:58:06 PST
(In reply to Yusuke Suzuki from comment #52)
> Is Promise and Promise.prototype safe?
> If we have a promise which holds a sensitive value, this leaks it.

IIRC (I need to check), async iterator uses promises and they have sensitive value in it.
Comment 54 Devin Rousso 2018-12-04 15:13:32 PST
(In reply to Yusuke Suzuki from comment #53)
> (In reply to Yusuke Suzuki from comment #52)
> > Is Promise and Promise.prototype safe?
> > If we have a promise which holds a sensitive value, this leaks it.
> 
> IIRC (I need to check), async iterator uses promises and they have sensitive value in it.
Is there anything that someone could do with a Promise returned by `queryObjects(Promise)` that would cause issues?  AFAIU, there's nothing in JS that would allow you to modify a Promise, other than adding a new Promise to its chain.  Am I missing an edge-case or new API that would let a developer "mess" with a Promise?

Most (if not all) of these values are accessible via WebInspector's heap snapshots already, so I don't think we're exposing things we wouldn't have already been exposing.  Yes, it may be easier to access these objects now (e.g. you wouldn't need to take a heap snapshot), but I think the utility of being able to query for things like this is far greater.

I'm not against the idea of also making Promise a disallowed type, similar to Function/Object/Array/Map/Set/Proxy, but I think that being able to `queryObject(Promise)` would be pretty useful.
Comment 55 Saam Barati 2018-12-04 15:17:24 PST
Comment on attachment 356300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356300&action=review

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:658
> +    JSValue constructorPrototype = object->get(exec, vm.propertyNames->prototype);

Do we care about this doing effects?
Comment 56 Saam Barati 2018-12-04 15:24:34 PST
Comment on attachment 356300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356300&action=review

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:640
> +JSValue JSInjectedScriptHost::queryObjects(ExecState* exec)

I'm still worried this will leak internal objects.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:656
> +    JSValue prototype = object;

Why not JSObject*?

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:666
> +    if (object->inherits<ProxyObject>(vm) || prototype.inherits<ProxyObject>(vm))
> +        return throwTypeError(exec, scope, "queryObjects cannot be called with a Proxy."_s);

This doesn't actually check the entire prototype chain.
Comment 57 Yusuke Suzuki 2018-12-04 18:07:11 PST
(In reply to Devin Rousso from comment #54)
> (In reply to Yusuke Suzuki from comment #53)
> > (In reply to Yusuke Suzuki from comment #52)
> > > Is Promise and Promise.prototype safe?
> > > If we have a promise which holds a sensitive value, this leaks it.
> > 
> > IIRC (I need to check), async iterator uses promises and they have sensitive value in it.
> Is there anything that someone could do with a Promise returned by
> `queryObjects(Promise)` that would cause issues?  AFAIU, there's nothing in
> JS that would allow you to modify a Promise, other than adding a new Promise
> to its chain.  Am I missing an edge-case or new API that would let a
> developer "mess" with a Promise?
> 
> Most (if not all) of these values are accessible via WebInspector's heap
> snapshots already, so I don't think we're exposing things we wouldn't have
> already been exposing.  Yes, it may be easier to access these objects now
> (e.g. you wouldn't need to take a heap snapshot), but I think the utility of
> being able to query for things like this is far greater.
> 
> I'm not against the idea of also making Promise a disallowed type, similar
> to Function/Object/Array/Map/Set/Proxy, but I think that being able to
> `queryObject(Promise)` would be pretty useful.

The  threat model in my mind is that, we have a Promise, which is fullfilled with an internal object, internal symbol etc.
If we can get this promise, we can get an internal object.

`promise.then(function(leaked) { /* do whatever we want with leaked value */ })`
Comment 58 Devin Rousso 2019-01-02 17:55:34 PST
Created attachment 358232 [details]
Patch
Comment 59 Saam Barati 2019-01-02 18:03:12 PST
Comment on attachment 358232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358232&action=review

LGTM

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:690
> +    vm.heap.collectNow(Sync, CollectionScope::Full);

We don't need to do this now, but perhaps we should call something collectNowFullIfNotDoneRecently in the future. So repeated calls doesn't cause a full GC every time.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:702
> +            JSValue value(static_cast<JSCell*>(cell));
> +            if (value.inherits<ProxyObject>(vm))

FWIW, this only catches Proxy to some degree. If I did:
```
let o = {};
let p = new Proxy({}, {getPrototypeOf(...args) { print("hello"); }})
o.__proto__ = p;
o instanceof function(){}
```
will print "hello"
Comment 60 Devin Rousso 2019-01-02 18:36:00 PST
Comment on attachment 358232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=358232&action=review

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:702
>> +            if (value.inherits<ProxyObject>(vm))
> 
> FWIW, this only catches Proxy to some degree. If I did:
> ```
> let o = {};
> let p = new Proxy({}, {getPrototypeOf(...args) { print("hello"); }})
> o.__proto__ = p;
> o instanceof function(){}
> ```
> will print "hello"

Frankly, I'm only really worried about a `Proxy` being passed in as `prototypeOrConstructor` as a means of circumventing our disallowed types.  If we iterate to an object that inherits from a `Proxy` and list that object in the result array, that's fine by me.
Comment 61 Saam Barati 2019-01-02 19:16:03 PST
(In reply to Devin Rousso from comment #60)
> Comment on attachment 358232 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358232&action=review
> 
> >> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:702
> >> +            if (value.inherits<ProxyObject>(vm))
> > 
> > FWIW, this only catches Proxy to some degree. If I did:
> > ```
> > let o = {};
> > let p = new Proxy({}, {getPrototypeOf(...args) { print("hello"); }})
> > o.__proto__ = p;
> > o instanceof function(){}
> > ```
> > will print "hello"
> 
> Frankly, I'm only really worried about a `Proxy` being passed in as
> `prototypeOrConstructor` as a means of circumventing our disallowed types. 
> If we iterate to an object that inherits from a `Proxy` and list that object
> in the result array, that's fine by me.

Sounds good.
Comment 62 WebKit Commit Bot 2019-01-02 19:48:27 PST
Comment on attachment 358232 [details]
Patch

Clearing flags on attachment: 358232

Committed r239583: <https://trac.webkit.org/changeset/239583>
Comment 63 WebKit Commit Bot 2019-01-02 19:48:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 64 Truitt Savell 2019-01-04 10:40:52 PST
Looks like the test added in https://trac.webkit.org/changeset/239583/webkit

inspector/console/queryObjects.html

is a flakey timeout on Debug

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fconsole%2FqueryObjects.html