Bug 107951 - Web Inspector: [Canvas] support instrumenting canvases in iframes (backend side)
Summary: Web Inspector: [Canvas] support instrumenting canvases in iframes (backend side)
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: Andrey Adaikin
URL:
Keywords:
Depends on:
Blocks: 108064 108319
  Show dependency treegraph
 
Reported: 2013-01-25 07:36 PST by Andrey Adaikin
Modified: 2013-01-30 03:08 PST (History)
12 users (show)

See Also:


Attachments
Patch (26.98 KB, patch)
2013-01-25 07:47 PST, Andrey Adaikin
no flags Details | Formatted Diff | Diff
Patch (20.70 KB, patch)
2013-01-28 05:18 PST, Andrey Adaikin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Adaikin 2013-01-25 07:36:51 PST
Accept optional FrameId argument for captureFrame and startCapturing commands. Add event to the protocol to inform about instrumented canvas context creation.

Patch to follow.
Comment 1 Andrey Adaikin 2013-01-25 07:47:13 PST
Created attachment 184750 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-25 08:40:27 PST
Comment on attachment 184750 [details]
Patch

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

New failing tests:
inspector/profiler/canvas2d/canvas2d-profiler-capturing-basics.html
inspector/profiler/canvas2d/canvas-has-uninstrumented-canvases.html
inspector/profiler/canvas2d/canvas-stack-trace.html
inspector/profiler/webgl/webgl-profiler-get-error.html
inspector/profiler/canvas2d/canvas2d-gradient-capturing.html
Comment 3 Build Bot 2013-01-25 10:38:43 PST
Comment on attachment 184750 [details]
Patch

Attachment 184750 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16120399

New failing tests:
inspector/profiler/canvas2d/canvas2d-profiler-capturing-basics.html
inspector/profiler/webgl/webgl-profiler-get-error.html
inspector/profiler/canvas2d/canvas2d-gradient-capturing.html
Comment 4 Build Bot 2013-01-25 11:32:00 PST
Comment on attachment 184750 [details]
Patch

Attachment 184750 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16122431

New failing tests:
inspector/profiler/canvas2d/canvas2d-profiler-capturing-basics.html
inspector/profiler/webgl/webgl-profiler-get-error.html
inspector/profiler/canvas2d/canvas2d-gradient-capturing.html
Comment 5 Pavel Feldman 2013-01-28 01:16:16 PST
Comment on attachment 184750 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:3382
> +                "name": "frameWithCanvases",

Consider renaming - event should tell us what exactly is happening

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
> +            if (m_framesWithUninstrumentedCanvases.contains(frame)) {

What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:190
> +    InjectedScriptCanvasModule module = injectedScriptCanvasModule(errorString, traceLogId);

This nice refactoring seems to be a separate change.
Comment 6 Andrey Adaikin 2013-01-28 01:22:34 PST
Comment on attachment 184750 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
>> +            if (m_framesWithUninstrumentedCanvases.contains(frame)) {
> 
> What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup.

I do track frameNavigated and frameDetached. Is this not enough?
Comment 7 Pavel Feldman 2013-01-28 01:32:56 PST
Comment on attachment 184750 [details]
Patch

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

>>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
>>> +            if (m_framesWithUninstrumentedCanvases.contains(frame)) {
>> 
>> What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup.
> 
> I do track frameNavigated and frameDetached. Is this not enough?

Why is the comment on no defering then?
Comment 8 Andrey Adaikin 2013-01-28 04:07:26 PST
(In reply to comment #7)
> (From update of attachment 184750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184750&action=review
> 
> >>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
> >>> +            if (m_framesWithUninstrumentedCanvases.contains(frame)) {
> >> 
> >> What if the frame was destroyed and re-created at same address? You should track frame's lifetime and make a cleanup.
> > 
> > I do track frameNavigated and frameDetached. Is this not enough?
> 
> Why is the comment on no defering then?

I'm just being too cautious (maybe without a good reason here) not to use-after-free. I dont want to deref a pointer, since I dont know it's ownership policy.

Is it safe to deref a Frame*, provided that I remove pointers on frameDetached?
Comment 9 Andrey Adaikin 2013-01-28 05:18:33 PST
Created attachment 184972 [details]
Patch
Comment 10 Pavel Feldman 2013-01-29 03:04:44 PST
Comment on attachment 184972 [details]
Patch

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

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:284
>      ScriptProfiler::visitNodeWrappers(&nodeVisitor);

What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll?
Comment 11 Andrey Adaikin 2013-01-29 03:17:42 PST
(In reply to comment #10)
> (From update of attachment 184972 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184972&action=review
> 
> > Source/WebCore/inspector/InspectorCanvasAgent.cpp:284
> >      ScriptProfiler::visitNodeWrappers(&nodeVisitor);
> 
> What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll?

I want those canvases as well as ones that are not attached to the DOM tree (yet).
Comment 12 Pavel Feldman 2013-01-29 03:34:10 PST
Comment on attachment 184972 [details]
Patch

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

>>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:284
>>>      ScriptProfiler::visitNodeWrappers(&nodeVisitor);
>> 
>> What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll?
> 
> I want those canvases as well as ones that are not attached to the DOM tree (yet).

Why would you want that?
Comment 13 Andrey Adaikin 2013-01-29 03:46:32 PST
(In reply to comment #12)
> (From update of attachment 184972 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184972&action=review
> 
> >>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:284
> >>>      ScriptProfiler::visitNodeWrappers(&nodeVisitor);
> >> 
> >> What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll?
> > 
> > I want those canvases as well as ones that are not attached to the DOM tree (yet).
> 
> Why would you want that?

1) To instrument canvases that are not in the DOM. It's quite common to create a temp canvas to render some part of the scene and then discard it.

2) To handle the case when we would enable CanvasAgent between a canvas context is created and it is appended to the DOM.
Comment 14 Pavel Feldman 2013-01-29 04:34:00 PST
Comment on attachment 184972 [details]
Patch

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

>>>>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:284
>>>>>      ScriptProfiler::visitNodeWrappers(&nodeVisitor);
>>>> 
>>>> What are you trying to do here? You should not depend on profiler. Do you want to traverse frame tree and collect all the canvases using querySelectorAll?
>>> 
>>> I want those canvases as well as ones that are not attached to the DOM tree (yet).
>> 
>> Why would you want that?
> 
> 1) To instrument canvases that are not in the DOM. It's quite common to create a temp canvas to render some part of the scene and then discard it.
> 
> 2) To handle the case when we would enable CanvasAgent between a canvas context is created and it is appended to the DOM.

Iterating over bindings to achieve that sounds like an overkill. It is a shame we need to do that.
Comment 15 WebKit Review Bot 2013-01-29 04:45:17 PST
Comment on attachment 184972 [details]
Patch

Clearing flags on attachment: 184972

Committed r141098: <http://trac.webkit.org/changeset/141098>
Comment 16 WebKit Review Bot 2013-01-29 04:45:22 PST
All reviewed patches have been landed.  Closing bug.