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).
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.
Attachment 375598 [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 157 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 375607 [details] [Patch] WIP
Attachment 375607 [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 157 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 375607 [details] [Patch] WIP Attachment 375607 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12868016 New failing tests: http/tests/inspector/dom/cross-domain-inspected-node-access.html
Created attachment 375614 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
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 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!
Created attachment 375708 [details] [Patch] WIP
Attachment 375708 [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 159 files If any of these errors are false positives, please file a bug against check-webkit-style.
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?
Created attachment 375761 [details] [Patch] WIP
Attachment 375761 [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 159 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 375789 [details] Patch
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.
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 on attachment 375789 [details] Patch Attachment 375789 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12878575 New failing tests: inspector/console/queryHolders.html
Created attachment 375797 [details] Archive of layout-test-results from ews113 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
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.
Attachment 376098 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127: 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 on attachment 376098 [details] Patch Attachment 376098 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12900264 New failing tests: http/tests/inspector/dom/cross-domain-inspected-node-access.html
Created attachment 376102 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376098 [details] Patch Attachment 376098 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12900324 New failing tests: http/tests/inspector/dom/cross-domain-inspected-node-access.html
Created attachment 376110 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376098 [details] Patch Attachment 376098 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12900328 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla
Created attachment 376117 [details] Patch
Attachment 376117 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127: METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 166 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 376118 [details] Patch Remove unnecessary logs in test.
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)?
Attachment 376118 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127: METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 166 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376118 [details] Patch Attachment 376118 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12901478 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla
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 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).
Created attachment 376464 [details] Patch
Attachment 376464 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127: METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:778: Extra space before [. [whitespace/brackets] [5] Total errors found: 2 in 166 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376464 [details] Patch Attachment 376464 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12922033 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl
> > >> 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 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 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.
Created attachment 376747 [details] Patch
Attachment 376747 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127: METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:778: Extra space before [. [whitespace/brackets] [5] Total errors found: 2 in 166 files If any of these errors are false positives, please file a bug against check-webkit-style.
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
> 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).
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).
Attachment 376790 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127: METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 167 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 376815 [details] Patch
Attachment 376815 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/ClassInfo.h:127: METHOD_TABLE_ENTRY is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 167 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376815 [details] Patch Clearing flags on attachment: 376815 Committed r248925: <https://trac.webkit.org/changeset/248925>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54536123>