Bug 61179 – Web Inspector: [Chromium] Add an ability to look up and explore an object from a heap profile

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Mikhail Naganov]

As not all properties can be stored in heap profiles (esp. for DOM wrappers), it is very important to be able to take an object and explore it in the console.

[Mikhail Naganov]

Created attachment 104521 [details]
WIP: A prototype implementation

[Pavel Feldman]

Comment on attachment 104521 [details]
WIP: A prototype implementation

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

Overall looks good. Could you please get rid of the copypaste and we can give it green light.

> Source/WebCore/inspector/Inspector.json:1840
> +                    { "name": "objectId", "type": "integer" }

You should probably start expanding your type definitions in this JSON.

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:273
> +        if (this._type === "string") {

This looks like a nasty copypaste. You could either create a new class ObjectPopover.js (preferred) or place this code into the ObjectPropertiesSection.

> Source/WebCore/inspector/front-end/DetailedHeapshotView.js:1048
> +        var element = hover[0];

Not nice

[Mikhail Naganov]

*** Bug 61176 has been marked as a duplicate of this bug. ***

[Mikhail Naganov]

Created attachment 109160 [details]
Patch

Might not compile on the cr-linux bot, as the new V8 version hasn't been pushed yet.

Also, contains 3 style violations:

Source/WebCore/inspector/InspectorProfilerAgent.cpp:382:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
-- These two can't be fixed, as in Inspector RPC we pass results this way. --

Source/WebCore/bindings/v8/ScriptProfiler.cpp:92:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
-- Results of 'if' branches are different types, so '?:' can't be used. --

[WebKit Review Bot]

Attachment 109160 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorProfilerAgent.cpp:382:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/ScriptProfiler.cpp:92:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 109160 [details]
Patch

Attachment 109160 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9900083

[Gustavo Noronha (kov)]

Comment on attachment 109160 [details]
Patch

Attachment 109160 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9893332

[Mikhail Naganov]

Created attachment 109162 [details]
Add JSC stubs

[WebKit Review Bot]

Attachment 109162 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorProfilerAgent.cpp:382:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/ScriptProfiler.cpp:92:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 109162 [details]
Add JSC stubs

Attachment 109162 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9880917

[Gyuyoung Kim]

Comment on attachment 109162 [details]
Add JSC stubs

Attachment 109162 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9889387

[Mikhail Naganov]

Created attachment 109163 [details]
JSC fix again

[WebKit Review Bot]

Attachment 109163 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorProfilerAgent.cpp:382:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/ScriptProfiler.cpp:92:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 109163 [details]
JSC fix again

Attachment 109163 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9898134

[Pavel Feldman]

Comment on attachment 109163 [details]
JSC fix again

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

Overall looks good. A number of nits to be fixed prior to landing.

> Source/WebCore/bindings/js/ScriptProfiler.cpp:45
> +PassRefPtr<InspectorValue> ScriptProfiler::getObjectByHeapObjectId(unsigned, InjectedScriptManager*)

While we use "get" prefixes in the protocol, regular WebCore classes should not have those.

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:83
> +    v8::HandleScope scope;

Blank line here would be great.

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:87
> +    v8::Handle<v8::Object> object(value.As<v8::Object>());

ditto

>> Source/WebCore/bindings/v8/ScriptProfiler.cpp:92
>> +    if (!injectedScript.hasNoValue())
> 
> An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]

Please follow the suggested style.

> Source/WebCore/inspector/Inspector.json:1947
> +                    { "name": "objectId", "type": "integer" }

You should define class for your ids in the long run.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:384
> +    if (m_frontend) {

no need to check for front-end.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:386
> +        heapObject->asObject(result);

Since this result is not optional (according to the protocol), you can't pass null here. You should assign errorString.

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:189
> +    } else if (node.flags & tree.snapshot.nodeFlags.canBeQueried) {

no need for {}

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:274
> +        if (this._type === "string") {

no need for {}

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:275
> +            callback({ type: "string", description: this._name });

Use WebInspector.RemoteObject.fromPrimitiveValue(this._name)

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:279
> +                if (object.type)

You should check for the error here.

> Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:282
> +                    callback({ type: "null", description: WebInspector.UIString("Not available") });

ditto

> Source/WebCore/inspector/front-end/HeapSnapshot.js:995
> +        var list = [];

A test for this method would be great.

> Source/WebCore/inspector/front-end/Popover.js:292
> +WebInspector.ObjectPopoverHelper = function(panelElement, getAnchor, queryObject, onHide, disableOnClick)

Please extract this in a separate change.

[Mikhail Naganov]

(In reply to comment #15)
> (From update of attachment 109163 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109163&action=review
> 
> Overall looks good. A number of nits to be fixed prior to landing.
> 
> > Source/WebCore/bindings/js/ScriptProfiler.cpp:45
> > +PassRefPtr<InspectorValue> ScriptProfiler::getObjectByHeapObjectId(unsigned, InjectedScriptManager*)
> 
> While we use "get" prefixes in the protocol, regular WebCore classes should not have those.
> 

Done.

> > Source/WebCore/bindings/v8/ScriptProfiler.cpp:83
> > +    v8::HandleScope scope;
> 
> Blank line here would be great.
>

Done.
 
> > Source/WebCore/bindings/v8/ScriptProfiler.cpp:87
> > +    v8::Handle<v8::Object> object(value.As<v8::Object>());
> 
> ditto
>

Done.
 
> >> Source/WebCore/bindings/v8/ScriptProfiler.cpp:92
> >> +    if (!injectedScript.hasNoValue())
> > 
> > An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
> 
> Please follow the suggested style.
> 

Done.

> > Source/WebCore/inspector/Inspector.json:1947
> > +                    { "name": "objectId", "type": "integer" }
> 
> You should define class for your ids in the long run.
>

Thanks, I'll consider that.
 
> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:384
> > +    if (m_frontend) {
> 
> no need to check for front-end.
> 

Done.

> > Source/WebCore/inspector/InspectorProfilerAgent.cpp:386
> > +        heapObject->asObject(result);
> 
> Since this result is not optional (according to the protocol), you can't pass null here. You should assign errorString.
>

Done.
 
> > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:189
> > +    } else if (node.flags & tree.snapshot.nodeFlags.canBeQueried) {
> 
> no need for {}
> 

Done.

> > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:274
> > +        if (this._type === "string") {
> 
> no need for {}
>

Done.

> > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:275
> > +            callback({ type: "string", description: this._name });
> 
> Use WebInspector.RemoteObject.fromPrimitiveValue(this._name)
> 

Done.

> > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:279
> > +                if (object.type)
> 
> You should check for the error here.
> 

Done.

> > Source/WebCore/inspector/front-end/DetailedHeapshotGridNodes.js:282
> > +                    callback({ type: "null", description: WebInspector.UIString("Not available") });
> 
> ditto
> 

Done.

> > Source/WebCore/inspector/front-end/HeapSnapshot.js:995
> > +        var list = [];
> 
> A test for this method would be great.
>

Adding...
 
> > Source/WebCore/inspector/front-end/Popover.js:292
> > +WebInspector.ObjectPopoverHelper = function(panelElement, getAnchor, queryObject, onHide, disableOnClick)
> 
> Please extract this in a separate change.

Extracted as https://bugs.webkit.org/show_bug.cgi?id=69234

[Mikhail Naganov]

Created attachment 109687 [details]
Comments addressed, test added

Still will not compile on cr-linux, V8 push should happen soon.

[WebKit Review Bot]

Attachment 109687 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/insp..." exit_code: 1

Source/WebCore/inspector/InspectorProfilerAgent.cpp:382:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorProfilerAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 109687 [details]
Comments addressed, test added

Attachment 109687 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9944529

[Mikhail Naganov]

Manually committed http://trac.webkit.org/changeset/97362


commit 3d65d067f74ec42ee5334fbf61de6a4731d001e2
Author: Mikhail Naganov <[e-mail]>
Date:   Wed Aug 3 15:28:43 2011 +0100

    Web Inspector: [Chromium] Add an ability to look up and explore an object from a heap profile.
    https://bugs.webkit.org/show_bug.cgi?id=61179
    
    This is exteremely helpful when dealing with DOM wrappers, as
    their properties are mostly implemented with getters and thus not
    stored in heap snapshots.
    
    Reviewed by Pavel Feldman.
    
    * English.lproj/localizedStrings.js:
    * bindings/js/ScriptProfiler.cpp:
    (WebCore::ScriptProfiler::objectByHeapObjectId):
    * bindings/js/ScriptProfiler.h:
    * bindings/v8/ScriptProfiler.cpp:
    (WebCore::ScriptProfiler::objectByHeapObjectId):
    * bindings/v8/ScriptProfiler.h:
    * inspector/Inspector.json:
    * inspector/InspectorController.cpp:
    (WebCore::InspectorController::InspectorController):
    * inspector/InspectorProfilerAgent.cpp:
    (WebCore::InspectorProfilerAgent::create):
    (WebCore::InspectorProfilerAgent::InspectorProfilerAgent):
    (WebCore::InspectorProfilerAgent::getObjectByHeapObjectId):
    * inspector/InspectorProfilerAgent.h:
    * inspector/front-end/DetailedHeapshotGridNodes.js:
    (WebInspector.HeapSnapshotGridNode.prototype.hasHoverMessage.false.queryObjectContent):
    (WebInspector.HeapSnapshotGenericObjectNode):
    (WebInspector.HeapSnapshotGenericObjectNode.prototype.get data):
    (WebInspector.HeapSnapshotGenericObjectNode.prototype.queryObjectContent.else.formatResult):
    (WebInspector.HeapSnapshotGenericObjectNode.prototype.queryObjectContent):
    (WebInspector.HeapSnapshotGenericObjectNode.prototype.shortenWindowURL):
    * inspector/front-end/DetailedHeapshotView.js:
    (WebInspector.DetailedHeapshotView.prototype._showObjectPopover):
    * inspector/front-end/HeapSnapshot.js:
    (WebInspector.HeapSnapshotNode.prototype.get canBeQueried):
    (WebInspector.HeapSnapshotNode.prototype.get flags):
    (WebInspector.HeapSnapshotNode.prototype.get isDOMWindow):
    (WebInspector.HeapSnapshot.prototype._init):
    (WebInspector.HeapSnapshot.prototype.dispose):
    (WebInspector.HeapSnapshot.prototype._flagsOfNode):
    (WebInspector.HeapSnapshot.prototype._calculateFlags):
    (WebInspector.HeapSnapshot.prototype.updateStaticData):
    (WebInspector.HeapSnapshotNodesProvider.prototype._serialize):
    * inspector/front-end/HeapSnapshotProxy.js:
    (WebInspector.HeapSnapshotProxy.prototype.get nodeFlags):
    * inspector/front-end/RemoteObject.js:
    (WebInspector.RemoteObject.fromError):
    * inspector/front-end/heapProfiler.css:
    (.detailed-heapshot-view tr:not(.selected) td.object-column span.highlight):
    
    * inspector/profiler/heap-snapshot-expected.txt:
    * inspector/profiler/heap-snapshot-test.js:
    (initialize_HeapSnapshotTest.InspectorTest.createHeapSnapshotMockWithDOM):
    (initialize_HeapSnapshotTest):
    * inspector/profiler/heap-snapshot.html:

[Dimitri Glazkov (Google)]

Sorry guys, had to roll it out, since it was using the API in V8 and that has been reverted: http://trac.webkit.org/changeset/97372

[Mikhail Naganov]

Re-applied again as http://trac.webkit.org/changeset/97611

V8 has been pushed w/o the new GC, but with the required API.

[Ilya Tikhonovsky]

Comment on attachment 109687 [details]
Comments addressed, test added

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

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:79
> +    // As ids are unique, it doesn't matter which HeapSnapshot owns HeapGraphNode.
> +    // We need to find first HeapSnapshot containing a node with the specified id.
> +    const v8::HeapGraphNode* node = 0;
> +    for (int i = 0, l = v8::HeapProfiler::GetSnapshotsCount(); i < l; ++i) {
> +        const v8::HeapSnapshot* snapshot = v8::HeapProfiler::GetSnapshot(i);
> +        node = snapshot->GetNodeById(id);
> +        if (node)
> +            break;
> +    }

The complexity of this method is N*ln2(K) where N is the number of snapshots and K is the number of nodes.
As far as Id is a monotonic function you can have a maxId property in the each snapshot and find the object with complexity ln2(N) + ln2(K)

[Mikhail Naganov]

(In reply to comment #23)
> (From update of attachment 109687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109687&action=review
> 
> > Source/WebCore/bindings/v8/ScriptProfiler.cpp:79
> > +    // As ids are unique, it doesn't matter which HeapSnapshot owns HeapGraphNode.
> > +    // We need to find first HeapSnapshot containing a node with the specified id.
> > +    const v8::HeapGraphNode* node = 0;
> > +    for (int i = 0, l = v8::HeapProfiler::GetSnapshotsCount(); i < l; ++i) {
> > +        const v8::HeapSnapshot* snapshot = v8::HeapProfiler::GetSnapshot(i);
> > +        node = snapshot->GetNodeById(id);
> > +        if (node)
> > +            break;
> > +    }
> 
> The complexity of this method is N*ln2(K) where N is the number of snapshots and K is the number of nodes.
> As far as Id is a monotonic function you can have a maxId property in the each snapshot and find the object with complexity ln2(N) + ln2(K)

Right, but N isn't big usually. Sure, we can optimize it anyway.

[Ilya Tikhonovsky]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 109687 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109687&action=review
> > 
> > > Source/WebCore/bindings/v8/ScriptProfiler.cpp:79
> > > +    // As ids are unique, it doesn't matter which HeapSnapshot owns HeapGraphNode.
> > > +    // We need to find first HeapSnapshot containing a node with the specified id.
> > > +    const v8::HeapGraphNode* node = 0;
> > > +    for (int i = 0, l = v8::HeapProfiler::GetSnapshotsCount(); i < l; ++i) {
> > > +        const v8::HeapSnapshot* snapshot = v8::HeapProfiler::GetSnapshot(i);
> > > +        node = snapshot->GetNodeById(id);
> > > +        if (node)
> > > +            break;
> > > +    }
> > 
> > The complexity of this method is N*ln2(K) where N is the number of snapshots and K is the number of nodes.
> > As far as Id is a monotonic function you can have a maxId property in the each snapshot and find the object with complexity ln2(N) + ln2(K)
> 
> Right, but N isn't big usually. Sure, we can optimize it anyway.

I'd like to see this property in the snapshots just for minimize the complexity in my patch :)