Bug 176766 – Web Inspector: Implement `queryObjects` Command Line API

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Saam Barati]

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.

[Sam Weinig]

Should it be in https://console.spec.whatwg.org?

[Joseph Pecoraro]

(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

[Sam Weinig]

(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.

[Devin Rousso]

Created attachment 321026 [details]
[Patch] WIP - missing tests

[Build Bot]

Comment on attachment 321026 [details]
[Patch] WIP - missing tests

Attachment 321026 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4573070

New failing tests:
http/tests/inspector/console/cross-domain-inspected-node-access.html

[Build Bot]

Created attachment 321032 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321026 [details]
[Patch] WIP - missing tests

Attachment 321026 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4573079

New failing tests:
http/tests/inspector/console/cross-domain-inspected-node-access.html

[Build Bot]

Created attachment 321034 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321026 [details]
[Patch] WIP - missing tests

Attachment 321026 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4573190

New failing tests:
http/tests/inspector/console/cross-domain-inspected-node-access.html

[Build Bot]

Created attachment 321037 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

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.

[Devin Rousso]

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).

[Build Bot]

Comment on attachment 321185 [details]
Patch

Attachment 321185 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4591736

New failing tests:
inspector/console/queryObjects.html

[Build Bot]

Created attachment 321187 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 321223 [details]
Patch

[Devin Rousso]

Created attachment 321372 [details]
Patch

[Joseph Pecoraro]

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.

[Saam Barati]

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.

[Joseph Pecoraro]

(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.

[Devin Rousso]

Created attachment 321519 [details]
Patch

[Joseph Pecoraro]

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();
                });
            });
        }
    });

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 321618 [details]
Patch

[Build Bot]

Comment on attachment 321618 [details]
Patch

Attachment 321618 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4634891

New failing tests:
http/tests/cache-storage/cache-representation.https.html

[Build Bot]

Created attachment 321621 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 321623 [details]
Patch

[Joseph Pecoraro]

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."

[Saam Barati]

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.

[Devin Rousso]

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?

[Devin Rousso]

Created attachment 322029 [details]
Patch

[Devin Rousso]

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.

[Blaze Burg]

(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?

[Radar WebKit Bug Importer]

<rdar://problem/34890688>

[Radar WebKit Bug Importer]

<rdar://problem/34890689>

[Saam Barati]

(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.

[Saam Barati]

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

[Devin Rousso]

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.

[Saam Barati]

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; } });
```

[Joseph Pecoraro]

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.

[Devin Rousso]

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

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 356288 [details]
Patch

Oops.  Looks like webkit-patch doesn't consider untracked files :/

[Devin Rousso]

Created attachment 356290 [details]
Patch

[EWS Watchlist]

Comment on attachment 356290 [details]
Patch

Attachment 356290 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10226073

New failing tests:
inspector/console/queryObjects.html

[EWS Watchlist]

Created attachment 356295 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 356300 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 356300 [details]
Patch

r=me, Though you should get a JSC engineer (Saam?) to sign off on this as well.

[Yusuke Suzuki]

Is Promise and Promise.prototype safe?
If we have a promise which holds a sensitive value, this leaks it.

[Yusuke Suzuki]

(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.

[Devin Rousso]

(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.

[Saam Barati]

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?

[Saam Barati]

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.

[Yusuke Suzuki]

(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 */ })`

[Devin Rousso]

Created attachment 358232 [details]
Patch

[Saam Barati]

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"

[Devin Rousso]

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.

[Saam Barati]

(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.

[WebKit Commit Bot]

Comment on attachment 358232 [details]
Patch

Clearing flags on attachment: 358232

Committed r239583: <https://trac.webkit.org/changeset/239583>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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