Bug 78496 - Web Inspector: [heap snapshot] It could be useful to have access to the selected heap object from the console.
Summary: Web Inspector: [heap snapshot] It could be useful to have access to the selec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks: 78411
  Show dependency treegraph
 
Reported: 2012-02-13 07:10 PST by Ilya Tikhonovsky
Modified: 2012-02-16 06:24 PST (History)
17 users (show)

See Also:


Attachments
Patch (12.13 KB, patch)
2012-02-13 07:26 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (16.44 KB, patch)
2012-02-15 00:56 PST, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (21.76 KB, patch)
2012-02-15 12:16 PST, Ilya Tikhonovsky
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2012-02-13 07:10:41 PST
The scenario:
0) make a heap snapshot.
1) select an object
2) open console
3) use it via $$0 property of DOMWindow
Comment 1 Ilya Tikhonovsky 2012-02-13 07:26:31 PST
Created attachment 126768 [details]
Patch
Comment 2 Yury Semikhatsky 2012-02-13 07:42:46 PST
Comment on attachment 126768 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:91
> +    ScriptObject object = host->inspectedObject(args[0]->ToInt32()->Value());

You should check for a null object and return a error/undefined here.

> Source/WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:92
> +    return object.v8Object();

The object may be from a context different from that where inspectedObjectCallback is called, in that case we should return undefined here to avoid object leaks between isolated contexts.

> Source/WebCore/inspector/InjectedScriptHost.cpp:152
> +    if (heapObject.hasNoValue())

No need to check this here.

> Source/WebCore/inspector/InjectedScriptSource.js:582
> +        this.__defineGetter__("$$" + i, bind(commandLineAPIImpl, commandLineAPIImpl._inspectedObject, i));

Let's use existing $* mechanism for addressing recently selected objects.
Comment 3 WebKit Review Bot 2012-02-13 08:27:16 PST
Comment on attachment 126768 [details]
Patch

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

New failing tests:
inspector/protocol/console-agent.html
Comment 4 Ilya Tikhonovsky 2012-02-15 00:56:56 PST
Created attachment 127128 [details]
Patch
Comment 5 Philippe Normand 2012-02-15 01:01:28 PST
Comment on attachment 127128 [details]
Patch

Attachment 127128 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11525317
Comment 6 Early Warning System Bot 2012-02-15 01:16:36 PST
Comment on attachment 127128 [details]
Patch

Attachment 127128 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11516611
Comment 7 Gyuyoung Kim 2012-02-15 01:21:50 PST
Comment on attachment 127128 [details]
Patch

Attachment 127128 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11515648
Comment 8 WebKit Review Bot 2012-02-15 02:03:30 PST
Comment on attachment 127128 [details]
Patch

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

New failing tests:
inspector/protocol/console-agent.html
inspector/extensions/extensions-sidebar.html
inspector/console/command-line-api.html
Comment 9 Yury Semikhatsky 2012-02-15 04:22:00 PST
Comment on attachment 127128 [details]
Patch

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

> Source/WebCore/inspector/InjectedScriptSource.js:690
> +        console.log(" _inspectedObject: " + num);

Remove this.

> Source/WebCore/inspector/Inspector.json:532
> +                "name": "addInspectedObject",

The method name is to generic, it should be clear that the object id is from heap profile.

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:286
> +    InspectableHeapObject(int heapObjectId) : m_heapObjectId(heapObjectId) { }

Should be marked explicit.

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:289
> +        return ScriptProfiler::objectByHeapObjectId(m_heapObjectId);

What if the object is from a different ScriptState?

> Source/WebCore/inspector/PageConsoleAgent.cpp:68
> +    InspectableNode(Node* node) : m_node(node) { }

Missing explicit.
Comment 10 Ilya Tikhonovsky 2012-02-15 12:16:29 PST
Created attachment 127212 [details]
Patch
Comment 11 Ilya Tikhonovsky 2012-02-15 12:19:03 PST
patch for bots
comments were addressed except about ScriptState
Comment 12 Yury Semikhatsky 2012-02-16 05:55:20 PST
Comment on attachment 127212 [details]
Patch

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

> Source/WebCore/ChangeLog:51
> +2012-02-15  Ilya Tikhonovsky  <loislo@chromium.org>

Please remove duplicate entry.

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:289
> +        return ScriptProfiler::objectByHeapObjectId(m_heapObjectId);

I think we need to reject objects from a different ScriptState here and when wrapping DOM Nodes. This can be done in a separate patch.
Comment 13 Ilya Tikhonovsky 2012-02-16 06:24:10 PST
Committed r107929: <http://trac.webkit.org/changeset/107929>