Bug 61179

Summary: Web Inspector: [Chromium] Add an ability to look up and explore an object from a heap profile
Product: WebKit Reporter: Mikhail Naganov <mnaganov>
Component: Web Inspector (Deprecated)Assignee: Mikhail Naganov <mnaganov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, dglazkov, gustavo, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 69234, 70039    
Bug Blocks:    
Attachments:
Description Flags
WIP: A prototype implementation
none
Patch
mnaganov: commit-queue-
Add JSC stubs
mnaganov: commit-queue-
JSC fix again
pfeldman: review-, mnaganov: commit-queue-
Comments addressed, test added pfeldman: review+, mnaganov: commit-queue-

Description Mikhail Naganov 2011-05-20 02:09:29 PDT
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.
Comment 1 Mikhail Naganov 2011-08-19 10:24:07 PDT
Created attachment 104521 [details]
WIP: A prototype implementation
Comment 2 Pavel Feldman 2011-08-19 12:11:26 PDT
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
Comment 3 Mikhail Naganov 2011-09-29 07:10:26 PDT
*** Bug 61176 has been marked as a duplicate of this bug. ***
Comment 4 Mikhail Naganov 2011-09-29 07:15:48 PDT
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. --
Comment 5 WebKit Review Bot 2011-09-29 07:17:07 PDT
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.
Comment 6 Gyuyoung Kim 2011-09-29 07:19:48 PDT
Comment on attachment 109160 [details]
Patch

Attachment 109160 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9900083
Comment 7 Gustavo Noronha (kov) 2011-09-29 07:22:38 PDT
Comment on attachment 109160 [details]
Patch

Attachment 109160 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9893332
Comment 8 Mikhail Naganov 2011-09-29 07:29:31 PDT
Created attachment 109162 [details]
Add JSC stubs
Comment 9 WebKit Review Bot 2011-09-29 07:31:55 PDT
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.
Comment 10 WebKit Review Bot 2011-09-29 07:34:29 PDT
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
Comment 11 Gyuyoung Kim 2011-09-29 07:35:35 PDT
Comment on attachment 109162 [details]
Add JSC stubs

Attachment 109162 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9889387
Comment 12 Mikhail Naganov 2011-09-29 07:39:38 PDT
Created attachment 109163 [details]
JSC fix again
Comment 13 WebKit Review Bot 2011-09-29 07:43:16 PDT
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.
Comment 14 WebKit Review Bot 2011-09-29 09:47:33 PDT
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
Comment 15 Pavel Feldman 2011-09-30 06:17:32 PDT
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.
Comment 16 Mikhail Naganov 2011-10-02 21:56:45 PDT
(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
Comment 17 Mikhail Naganov 2011-10-04 14:20:02 PDT
Created attachment 109687 [details]
Comments addressed, test added

Still will not compile on cr-linux, V8 push should happen soon.
Comment 18 WebKit Review Bot 2011-10-04 14:23:43 PDT
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.
Comment 19 WebKit Review Bot 2011-10-04 15:33:57 PDT
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
Comment 20 Mikhail Naganov 2011-10-13 04:09:59 PDT
Manually committed http://trac.webkit.org/changeset/97362


commit 3d65d067f74ec42ee5334fbf61de6a4731d001e2
Author: Mikhail Naganov <mnaganov@chromium.org>
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:
Comment 21 Dimitri Glazkov (Google) 2011-10-13 11:03:03 PDT
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
Comment 22 Mikhail Naganov 2011-10-17 06:22:08 PDT
Re-applied again as http://trac.webkit.org/changeset/97611

V8 has been pushed w/o the new GC, but with the required API.
Comment 23 Ilya Tikhonovsky 2011-10-17 06:32:18 PDT
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)
Comment 24 Mikhail Naganov 2011-10-17 06:37:59 PDT
(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.
Comment 25 Ilya Tikhonovsky 2011-10-17 06:42:18 PDT
(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 :)