Bug 107951 – Web Inspector: [Canvas] support instrumenting canvases in iframes (backend side)

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

[Andrey Adaikin]

Accept optional FrameId argument for captureFrame and startCapturing commands. Add event to the protocol to inform about instrumented canvas context creation.

Patch to follow.

[Andrey Adaikin]

Created attachment 184750 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Pavel Feldman]

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.

[Andrey Adaikin]

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?

[Pavel Feldman]

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?

[Andrey Adaikin]

(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?

[Andrey Adaikin]

Created attachment 184972 [details]
Patch

[Pavel Feldman]

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?

[Andrey Adaikin]

(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).

[Pavel Feldman]

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?

[Andrey Adaikin]

(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.

[Pavel Feldman]

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.

[WebKit Review Bot]

Comment on attachment 184972 [details]
Patch

Clearing flags on attachment: 184972

Committed r141098: <http://trac.webkit.org/changeset/141098>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.