Bug 195241 – Web Inspector: Canvas: lazily create the agent

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

Created attachment 363410 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/48532545>

[Darin Adler]

Comment on attachment 363410 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:53
> +    static Lock mutex;

I don’t understand this locking. If the work (creation and destroying canvas rendering contexts) is always done on the same thread, then we don’t need a lock at all. If the work is done on multiple threads, then we have a race if two thread both try to create this global the first time.

Typically DOM objects are main thread only, do I don’t understand the threading code here.

[EWS Watchlist]

Comment on attachment 363410 [details]
Patch

Attachment 363410 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11340620

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 363415 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Joseph Pecoraro]

Comment on attachment 363410 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:53
> -        return window.CanvasAgent && CanvasAgent.setRecordingAutoCaptureFrameCount;
> +        return InspectorBackend.domains.CanvasAgent && InspectorBackend.domains.CanvasAgent.setRecordingAutoCaptureFrameCount;

Nice!

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:106
> +        for (let target of WI.targets) {
> +            target.CanvasAgent.setRecordingAutoCaptureFrameCount(enabled ? count : 0)
> +            .then(() => {
> +                WI.settings.canvasRecordingAutoCaptureEnabled.value = enabled && count;
> +                WI.settings.canvasRecordingAutoCaptureFrameCount.value = count;
> +            });
> +        }

No need to update the result multiple times, just do it once. Also no need to do it only on success. See the comment in the other bug.
You should also check `if (target.CanvasAgent)` as you did in the other bug, since in the future not all agents will have this agent.

[Joseph Pecoraro]

Correction: in the future not all *targets* will have this agent.

[Devin Rousso]

Comment on attachment 363410 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:53
>> +    static Lock mutex;
> 
> I don’t understand this locking. If the work (creation and destroying canvas rendering contexts) is always done on the same thread, then we don’t need a lock at all. If the work is done on multiple threads, then we have a race if two thread both try to create this global the first time.
> 
> Typically DOM objects are main thread only, do I don’t understand the threading code here.

Canvases are not limited to the main thread.  OffscreenCanvas provides a way to create rendering contexts from Workers.  Other types of worklets (e.g. PaintWorklet) also expose rendering contexts.

Perhaps there is a better way of doing this change other than having a global list.  I'll rethink it a bit.

[Devin Rousso]

Created attachment 363481 [details]
Patch

Addressed Joe's responses from comment #6.  Still need to do as I'd said in comment #8.

[Devin Rousso]

Comment on attachment 363481 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:901
> +                entry.value = nullptr;

This deserves a comment explaining that we don't want to remove any `WebGLProgram` from the map, as they may not have been destructed yet (`~WebGLProgram` asserts that it is in the map).  As such, we should only be clearing the association between the `WebGLProgram` and the `WebGLRenderingContextBase`.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:453
>      const bool captureBacktrace = true;

I just realized that since we would now call `didCreateCanvasRenderingContext` inside `enable`, we also need to make sure we don't capture a backtrace.

[Darin Adler]

Comment on attachment 363481 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:54
> +    static Lock mutex;
> +    return mutex;

Isn’t there a race here if two threads both call this the first time, because WebKit is compiled with an option that turns off thread safety for initialization of globals like this? Or is Lock somehow immune to such concerns?

[Devin Rousso]

Created attachment 363531 [details]
Patch

[Devin Rousso]

Created attachment 363549 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 363549 [details]
Patch

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

Nice! r=me

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:123
> +                bindCanvas(*canvasRenderingContext, false);

Nit: For readability you could keep a:

    const bool captureBacktrace = false;

And use it here.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:140
> +    m_instrumentingAgents.setInspectorCanvasAgent(nullptr);

If you wanted you could bail if we're already disabled like so:

    if (!m_instrumentingAgents.inspectorCanvasAgent())
        return;

But our frontend doesn't even call disable, and it appears it would be harmless.

[Devin Rousso]

Comment on attachment 363549 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:123
>> +                bindCanvas(*canvasRenderingContext, false);
> 
> Nit: For readability you could keep a:
> 
>     const bool captureBacktrace = false;
> 
> And use it here.

I tend to only find those useful when the function isn't defined in the same file.  If it is, a simple ⌘E-⌘G will get me the answer.

[WebKit Commit Bot]

Comment on attachment 363549 [details]
Patch

Clearing flags on attachment: 363549

Committed r242594: <https://trac.webkit.org/changeset/242594>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.