Bug 195241 - Web Inspector: Canvas: lazily create the agent
Summary: Web Inspector: Canvas: lazily create the agent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 195825
  Show dependency treegraph
 
Reported: 2019-03-01 21:28 PST by Devin Rousso
Modified: 2019-03-15 15:13 PDT (History)
13 users (show)

See Also:


Attachments
Patch (21.49 KB, patch)
2019-03-01 21:31 PST, Devin Rousso
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (2.18 MB, application/zip)
2019-03-02 00:32 PST, Build Bot
no flags Details
Patch (18.94 KB, patch)
2019-03-03 21:03 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (21.18 KB, patch)
2019-03-04 11:38 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (21.51 KB, patch)
2019-03-04 13:33 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-03-01 21:28:28 PST
.
Comment 1 Devin Rousso 2019-03-01 21:31:19 PST
Created attachment 363410 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-03-01 21:31:34 PST
<rdar://problem/48532545>
Comment 3 Darin Adler 2019-03-01 22:03:32 PST
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.
Comment 4 Build Bot 2019-03-02 00:32:09 PST Comment hidden (obsolete)
Comment 5 Build Bot 2019-03-02 00:32:11 PST Comment hidden (obsolete)
Comment 6 Joseph Pecoraro 2019-03-02 01:48:56 PST
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.
Comment 7 Joseph Pecoraro 2019-03-02 01:49:36 PST
Correction: in the future not all *targets* will have this agent.
Comment 8 Devin Rousso 2019-03-03 21:00:13 PST
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.
Comment 9 Devin Rousso 2019-03-03 21:03:47 PST
Created attachment 363481 [details]
Patch

Addressed Joe's responses from comment #6.  Still need to do as I'd said in comment #8.
Comment 10 Devin Rousso 2019-03-03 21:09:54 PST
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.
Comment 11 Darin Adler 2019-03-04 08:56:44 PST
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?
Comment 12 Devin Rousso 2019-03-04 11:38:27 PST
Created attachment 363531 [details]
Patch
Comment 13 Devin Rousso 2019-03-04 13:33:58 PST
Created attachment 363549 [details]
Patch
Comment 14 Joseph Pecoraro 2019-03-06 22:34:17 PST
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.
Comment 15 Devin Rousso 2019-03-07 00:14:06 PST
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.
Comment 16 WebKit Commit Bot 2019-03-07 00:41:31 PST
Comment on attachment 363549 [details]
Patch

Clearing flags on attachment: 363549

Committed r242594: <https://trac.webkit.org/changeset/242594>
Comment 17 WebKit Commit Bot 2019-03-07 00:41:33 PST
All reviewed patches have been landed.  Closing bug.