Bug 200458

Summary: Web Inspector: Implement `queryHolders` Command Line API
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, drousso, ews, inspector-bugzilla-changes, joepeck, mark.lam, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200912
Attachments:
Description Flags
[Patch] WIP
drousso: commit-queue-
[Patch] WIP
drousso: commit-queue-
Archive of layout-test-results from ews100 for mac-highsierra
none
[Patch] WIP
drousso: commit-queue-
[Patch] WIP
none
Patch
none
Archive of layout-test-results from ews113 for mac-highsierra
none
Patch
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Devin Rousso 2019-08-05 16:44:38 PDT
Call queryRetainers(object) from the Console to return an array of objects that strongly reference the given `object`.

This would be extremely useful for finding specific leaks (understanding how a particular object is kept alive).
Comment 1 Devin Rousso 2019-08-05 21:07:11 PDT
Created attachment 375598 [details]
[Patch] WIP

This works, but it needs tests and a way of removing inspector objects (e.g. `InjectedScript`) from the list of retainers.
Comment 2 Build Bot 2019-08-05 21:10:09 PDT Comment hidden (obsolete)
Comment 3 Devin Rousso 2019-08-05 23:43:53 PDT
Created attachment 375607 [details]
[Patch] WIP
Comment 4 Build Bot 2019-08-05 23:45:05 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-08-06 01:07:58 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-08-06 01:08:00 PDT Comment hidden (obsolete)
Comment 7 Saam Barati 2019-08-06 15:04:42 PDT
Comment on attachment 375607 [details]
[Patch] WIP

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:769
> +    void appendNode(JSCell*) { }
> +    void appendPropertyNameEdge(JSCell* /* from */, JSCell* /* to */, UniquedStringImpl*) { }
> +    void appendVariableNameEdge(JSCell* /* from */, JSCell* /* to */, UniquedStringImpl*) { }
> +    void appendIndexEdge(JSCell* /* from */, JSCell* /* to */, uint32_t) { }
> +    void setOpaqueRootReachabilityReasonForCell(JSCell*, const char*) { }
> +    void setWrappedObjectForCell(JSCell*, void*) { }
> +    void setLabelForCell(JSCell*, const String&) { }

why do these do nothing? Is the above appendEdge guaranteed to be called too? Why not just not implement them?

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:790
> +    PreventCollectionScope preventCollectionScope(vm.heap);

I don't think this is doing exactly what you want. See below.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:797
> +        result->putDirectIndex(exec, result->length(), retainer);

In theory this can cause a collection, leading to the HashSet<JSCell*> pointing to things that will get collected.

I'd probably make do something where you run the first half of this as you do now, and then run this loop under DeferGC. Alternatively, you can transfer from a HashSet<JSCell*> to a MarkedArgumentBuffer and just iterate that.
Comment 8 Devin Rousso 2019-08-06 17:12:11 PDT
Comment on attachment 375607 [details]
[Patch] WIP

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

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:769
>> +    void setLabelForCell(JSCell*, const String&) { }
> 
> why do these do nothing? Is the above appendEdge guaranteed to be called too? Why not just not implement them?

I had originally thought that `appendEdge` (and also `appendNode`) were guaranteed to be called for every visited edge/node, and that the rest of the functions all get called during each object's `analyzeHeap` (formerly known as `buildSnapshot`) which happens after the GC finishes (`Heap::didFinishCollection`, which calls `Heap::gatherExtraHeapData`, which iterates the `m_objectSpace.forEachLiveCell`).

I now see that both are necessary, so I will add actual code to each of them.

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:797
>> +        result->putDirectIndex(exec, result->length(), retainer);
> 
> In theory this can cause a collection, leading to the HashSet<JSCell*> pointing to things that will get collected.
> 
> I'd probably make do something where you run the first half of this as you do now, and then run this loop under DeferGC. Alternatively, you can transfer from a HashSet<JSCell*> to a MarkedArgumentBuffer and just iterate that.

It seems like the other inspector heap related functions all lock the VM (which I don't think is necessary here, as this is a call coming from JavaScript, whereas other inspector things are coming from the inspector frontend) and `DeferGC` from the beginning, so I'll match that too.  Thanks for the head's up!
Comment 9 Devin Rousso 2019-08-07 10:19:50 PDT
Created attachment 375708 [details]
[Patch] WIP
Comment 10 Build Bot 2019-08-07 10:22:35 PDT Comment hidden (obsolete)
Comment 11 Joseph Pecoraro 2019-08-07 14:39:42 PDT
Comment on attachment 375708 [details]
[Patch] WIP

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

Nice. Makes sense and looks good to me! I like the HeapAnalyzer rename.

We could use a similar approach to mark public (non-inspector) objects in a normal HeapSnapshot so we can hide InjectedScriptSource (or display it differently) in the snapshot viewer.

Currently this could show internal objects that reference public objects like:

    promiseCapability.@promise = promise

I don't think this is a problem, but it is something to consider. We avoided this in queryObjects by just not returning Objects/Arrays.

> Source/JavaScriptCore/heap/HeapAnalyzer.h:45
> +    // A root or marked cell.
> +    virtual void analyzeNode(JSCell*) = 0;
> +
> +    // A reference from one cell to another.
> +    virtual void analyzeEdge(JSCell* from, JSCell* to, SlotVisitor::RootMarkReason) = 0;
> +    virtual void analyzePropertyNameEdge(JSCell* from, JSCell* to, UniquedStringImpl* propertyName) = 0;
> +    virtual void analyzeVariableNameEdge(JSCell* from, JSCell* to, UniquedStringImpl* variableName) = 0;
> +    virtual void analyzeIndexEdge(JSCell* from, JSCell* to, uint32_t index) = 0;

I don't particularly like these being renaming to `analyzeNode` it is more like `visitNode` / `includeEdge` but I guess its not that bad.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:761
> +        HashSet<JSCell*> queued;

We should really have a comment at this point about the remaining code.

    // Filter the list of retainers down to those reachable from non-Debugger roots.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:813
> +    HashSet<JSCell*> retainers;

Why isn't this a private `m_retainers` with a getter?
Comment 12 Devin Rousso 2019-08-07 16:10:38 PDT
Created attachment 375761 [details]
[Patch] WIP
Comment 13 Build Bot 2019-08-07 16:12:10 PDT Comment hidden (obsolete)
Comment 14 Devin Rousso 2019-08-08 01:41:11 PDT
Created attachment 375789 [details]
Patch
Comment 15 Devin Rousso 2019-08-08 01:42:22 PDT
Comment on attachment 375789 [details]
Patch

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

> LayoutTests/inspector/console/queryHolders.html:58
> +                    InspectorTest.expectEqual(remoteObject.type, "object", "The result should be an object.");
> +                    InspectorTest.expectEqual(remoteObject.subtype, "array", "The result should be an array object.");

These could probably just be `InspectorTest.assert` :/

> LayoutTests/inspector/console/queryHolders.html:127
> +                InspectorTest.expectEqual(remoteObject.type, "object", "The result should be an object.");
> +                InspectorTest.expectEqual(remoteObject.subtype, "array", "The result should be an array object.");

Ditto.
Comment 16 Build Bot 2019-08-08 01:43:45 PDT
Attachment 375789 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:124:  METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 165 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Build Bot 2019-08-08 05:37:26 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2019-08-08 05:37:28 PDT Comment hidden (obsolete)
Comment 19 Devin Rousso 2019-08-12 15:16:52 PDT
Created attachment 376098 [details]
Patch

Skip inspector/console/queryHolders.html for WK1 as heap snapshots are skewed since the inspector shares the same process as the inspected page.
Comment 20 Build Bot 2019-08-12 15:20:10 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2019-08-12 16:31:37 PDT Comment hidden (obsolete)
Comment 22 Build Bot 2019-08-12 16:31:40 PDT Comment hidden (obsolete)
Comment 23 Build Bot 2019-08-12 17:14:53 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2019-08-12 17:14:55 PDT Comment hidden (obsolete)
Comment 25 Build Bot 2019-08-12 17:34:46 PDT Comment hidden (obsolete)
Comment 26 Devin Rousso 2019-08-12 17:38:38 PDT
Created attachment 376117 [details]
Patch
Comment 27 Build Bot 2019-08-12 17:40:12 PDT Comment hidden (obsolete)
Comment 28 Devin Rousso 2019-08-12 17:46:21 PDT
Created attachment 376118 [details]
Patch

Remove unnecessary logs in test.
Comment 29 Devin Rousso 2019-08-12 17:47:59 PDT
Comment on attachment 376118 [details]
Patch

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

> LayoutTests/inspector/console/queryHolders-expected.txt:24
> +[Root, Symbol]

I wonder what this `Symbol` is?  I'd expect that the `Map` would also hold on to the `MapKey`, so maybe this is related to that?  If so, perhaps we should reach "up"/"back" one edge if the direct holder isn't an `Object` (e.g. the holders of the holder)?
Comment 30 Build Bot 2019-08-12 17:48:29 PDT Comment hidden (obsolete)
Comment 31 Build Bot 2019-08-12 21:18:37 PDT Comment hidden (obsolete)
Comment 32 Saam Barati 2019-08-15 17:41:30 PDT
Comment on attachment 376118 [details]
Patch

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

The JSC part looks good to me!

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:755
> +    {

this algorithm looks good to me.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:771
> +            if (!visited.add(from))

nit: I kinda like to query "isNewEntry" just for readability.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:782
> +    HashSet<JSCell*>&& releaseHolders() { return WTFMove(m_holders); }

nit: the way you're using this, I don't think you're ever actually releasing it, since it doesn't get moved into a copy or constructor. If you really do wanna "release" it, you can just return HashSet<JSCell*> instead of HashSet<JSCell*>&&.

However, in the context you're using it, you can just copy from a reference to this, since it's not important where you "release" it, since the only use is copying to that vector.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:784
> +    void analyzeEdge(JSCell* from, JSCell* to, SlotVisitor::RootMarkReason reason)

what does this do if from == to? I'm curious what queryHolders returns if an object points to itself.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:789
> +        std::lock_guard<Lock> lock(m_mutex);

nit: "auto locker = holdLock(m_mutex);"

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:834
> +    if (exec->argumentCount() < 1)
> +        return jsUndefined();
> +
> +    VM& vm = exec->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);
> +
> +    JSValue target = exec->uncheckedArgument(0);
> +    if (!target.isObject())
> +        return throwTypeError(exec, scope, "queryHolders first argument must be an object."_s);

why no type error if no argument is provided? Seems equally weird to providing a non-object.

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:839
> +    {

this looks solid to me.
Comment 33 Devin Rousso 2019-08-15 18:33:37 PDT
Comment on attachment 376118 [details]
Patch

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

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:782
>> +    HashSet<JSCell*>&& releaseHolders() { return WTFMove(m_holders); }
> 
> nit: the way you're using this, I don't think you're ever actually releasing it, since it doesn't get moved into a copy or constructor. If you really do wanna "release" it, you can just return HashSet<JSCell*> instead of HashSet<JSCell*>&&.
> 
> However, in the context you're using it, you can just copy from a reference to this, since it's not important where you "release" it, since the only use is copying to that vector.

That's true.  I usually like to be more "ideal" than what my code actually needs (e.g. once the holders are computed, the finder shouldn't have any reason to keep hold of the list), but there's no real reason for that.  I'll change it.

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:784
>> +    void analyzeEdge(JSCell* from, JSCell* to, SlotVisitor::RootMarkReason reason)
> 
> what does this do if from == to? I'm curious what queryHolders returns if an object points to itself.

I tried adding a `Root.Root = Root` line in the test file and call `queryHolders(Root)`, which just returned `Root`.  I think I should ignore `from == to` edges, as that isn't the "type" of holder that I think most would expect to be returned by `queryHolders` (e.g. a self reference doesn't "really" hold anything strongly).

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:834
>> +        return throwTypeError(exec, scope, "queryHolders first argument must be an object."_s);
> 
> why no type error if no argument is provided? Seems equally weird to providing a non-object.

This is typical of `console` and Inspector Console API functions.  We try to be as "accepting" as possible, so no arguments acts like you did nothing (this is how most of the `console.*` functions work too).  The only time we throw exceptions is if we're given something that's completely incompatible (e.g. argument isn't an object).
Comment 34 Devin Rousso 2019-08-15 19:05:10 PDT
Created attachment 376464 [details]
Patch
Comment 35 Build Bot 2019-08-15 19:07:48 PDT Comment hidden (obsolete)
Comment 36 Build Bot 2019-08-15 21:59:34 PDT Comment hidden (obsolete)
Comment 37 Joseph Pecoraro 2019-08-19 21:32:44 PDT
> 
> >> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:834
> >> +        return throwTypeError(exec, scope, "queryHolders first argument must be an object."_s);
> > 
> > why no type error if no argument is provided? Seems equally weird to providing a non-object.
> 
> This is typical of `console` and Inspector Console API functions.  We try to
> be as "accepting" as possible, so no arguments acts like you did nothing
> (this is how most of the `console.*` functions work too).  The only time we
> throw exceptions is if we're given something that's completely incompatible
> (e.g. argument isn't an object).

This is true of `console` APIs.

However for Command Line APIs we should probably be as strict as possible. They are only ever reachable when Web Inspector is open and a developer is therefore sitting right there using it. Errors can help them use the Command Line APIs correctly.
Comment 38 Joseph Pecoraro 2019-08-19 22:03:28 PDT
Comment on attachment 376464 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:808
> +        if (reason == SlotVisitor::RootMarkReason::Debugger)

Would be great to apply this to HeapSnapshotBuilder and make it so the Web Inspector UI can hide debugger objects!

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:335
>      "queryInstances",
>      "queryObjects",
> +    "queryHolders",

Look like this was in alphabetical order. Might as well keep it that way!

> LayoutTests/inspector/console/queryHolders-expected.txt:66
> +-- Running test case: CommandLineAPI.queryHolders.OnlyHeldByDebugger
> +PASS: The result should have 0 items.

I wonder if this will confuse anyone. I think an empty array is the right thing to do though.

> LayoutTests/inspector/console/queryHolders.html:97
> +    addTest(`PromiseFinally`);

Great tests!

Possible additional tests (not required):

  • An object only referenced via an Object Symbol property (should just be like an object string property)
  • An object with a lot of references, like `window` or something like that (ensure nothing crashes)
  • An object referenced in an array a lot of times only shows up once `arr = [foo, foo]`, foo has a single `arr` holder, since it is de-duplicated.

What happens with:

  • Elements. With `$0` do we see the parent element? I'd suspect it might just be the DOMWrapperWorld, which has no good output.

> LayoutTests/inspector/console/queryHolders.html:151
> +    suite.addTestCase({
> +        name: "CommandLineAPI.queryHolders.NoParameter",

Yeah, I kinda think `queryHolders()` and `queryObjects()` no parameters should throw an exception. Could be done as a follow-up. I realize that doesn't match what Chrome does.
Comment 39 Devin Rousso 2019-08-19 22:57:50 PDT
Comment on attachment 376464 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js:335
>> +    "queryHolders",
> 
> Look like this was in alphabetical order. Might as well keep it that way!

Ah oops!  Bad manual rebase merging on my part  :P

>> LayoutTests/inspector/console/queryHolders.html:97
>> +    addTest(`PromiseFinally`);
> 
> Great tests!
> 
> Possible additional tests (not required):
> 
>   • An object only referenced via an Object Symbol property (should just be like an object string property)
>   • An object with a lot of references, like `window` or something like that (ensure nothing crashes)
>   • An object referenced in an array a lot of times only shows up once `arr = [foo, foo]`, foo has a single `arr` holder, since it is de-duplicated.
> 
> What happens with:
> 
>   • Elements. With `$0` do we see the parent element? I'd suspect it might just be the DOMWrapperWorld, which has no good output.

I added three more tests:
 - object held via a `Symbol`
 - create 1000 different objects and have all of them reference the same object
 - create an object with 1000 references to the same object
All three tests behave as you'd expect (object with symbol property is shown, 1000 objects are shown, one object is shown).

As far as `$0` (or any node), the parent-child relationship doesn't appear to "carry over" to JS world.  `queryHolders($0)` actually returns `[]` (unless the underlying `$0` value is also referenced by a JS value).  This is true for any DOM node, though, and that somewhat makes sense, as the parent-child relationship is held by data inside WebCore, not JSC.  Making a DOM node referenced by a parent in any other way than parent-child (e.g. `parent.__foo = child`) _will_ cause the parent to appear in the result.
Comment 40 Devin Rousso 2019-08-19 22:59:14 PDT
Created attachment 376747 [details]
Patch
Comment 41 Build Bot 2019-08-19 23:02:19 PDT Comment hidden (obsolete)
Comment 42 Devin Rousso 2019-08-19 23:13:04 PDT
Comment on attachment 376464 [details]
Patch

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

>> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:808
>> +        if (reason == SlotVisitor::RootMarkReason::Debugger)
> 
> Would be great to apply this to HeapSnapshotBuilder and make it so the Web Inspector UI can hide debugger objects!

😁 <https://webkit.org/b/200912> Web Inspector: Heap: hide injected inspector objects from snapshots
Comment 43 Joseph Pecoraro 2019-08-20 00:56:56 PDT
> As far as `$0` (or any node), the parent-child relationship doesn't appear
> to "carry over" to JS world.  `queryHolders($0)` actually returns `[]`

Yeah, I think this would be a perfect opportunity to have an InjectedScriptHost virtual method for WebCore to override.

A possible additional holders would be:

  (1) If the JSValue is a Node, and the Node is attached to the DOM, include the `toJS(node.parent)`.
  (2) If the JSValue is a Node, include the `toJS(node.ownerDocument)`

I am kind of partial to (2). Users can already "Reveal in DOM Tree" for (1).
Comment 44 Devin Rousso 2019-08-20 12:32:59 PDT
Created attachment 376790 [details]
Patch

If a holder isn't an object, we should keep looking at successive predecessors (not just the immediate predecessor) for any objects, as there may be other non-object predecessors (e.g. `JSMap` may have multiple levels of `HashMapBucket` before the actual `key` object is reached).
Comment 45 Build Bot 2019-08-20 12:37:24 PDT Comment hidden (obsolete)
Comment 46 Devin Rousso 2019-08-20 15:04:10 PDT
Created attachment 376815 [details]
Patch
Comment 47 Build Bot 2019-08-20 15:06:42 PDT Comment hidden (obsolete)
Comment 48 WebKit Commit Bot 2019-08-20 17:24:36 PDT
Comment on attachment 376815 [details]
Patch

Clearing flags on attachment: 376815

Committed r248925: <https://trac.webkit.org/changeset/248925>
Comment 49 WebKit Commit Bot 2019-08-20 17:24:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 50 Radar WebKit Bug Importer 2019-08-20 17:25:23 PDT
<rdar://problem/54536123>