Bug 201650 - Web Inspector: Canvas: instrument WebGPUDevice instead of GPUCanvasContext
Summary: Web Inspector: Canvas: instrument WebGPUDevice instead of GPUCanvasContext
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: 201651 201675
  Show dependency treegraph
 
Reported: 2019-09-10 11:08 PDT by Devin Rousso
Modified: 2019-09-11 22:17 PDT (History)
19 users (show)

See Also:


Attachments
Patch (153.69 KB, patch)
2019-09-10 13:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (584.06 KB, image/png)
2019-09-10 18:09 PDT, Devin Rousso
no flags Details
Patch (159.30 KB, patch)
2019-09-10 18:12 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (159.31 KB, patch)
2019-09-10 19:36 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (159.25 KB, patch)
2019-09-11 15:53 PDT, 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-09-10 11:08:32 PDT
Most of the actual "work" done with Web GPU actually uses a `WebGPUDevice`.  A `GPUCanvasContext` is basically just a display "client" of the device, and isn't even required (e.g. compute pipeline).  We should treat the `GPUCanvasContext` almost like a `-webkit-canvas` client of a `WebGPUDevice`.

This change should also carry to the frontend (e.g. use "Device" instead of "Canvas"/"Context").
Comment 1 Devin Rousso 2019-09-10 13:32:46 PDT
Created attachment 378480 [details]
Patch
Comment 2 Build Bot 2019-09-10 13:33:49 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 3 Joseph Pecoraro 2019-09-10 16:36:42 PDT
Comment on attachment 378480 [details]
Patch

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

r=me - How does this look in the UI? Internals seem good.

> Source/JavaScriptCore/ChangeLog:8
> +        Most of the actual "work" done with Web GPU actually uses a `WebGPUDevice`.

These repeated details only make sense inside of the WebCore ChangeLog. Not JavaScriptCore / WebInspectorUI.

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:81
> +    ~WebGPUDevice();

virtual?

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:124
> +            if (context && existsInCurrentPage(context->canvasBase().scriptExecutionContext()))

Is this `context` check necessary? It seems unlikely that `CanvasRenderingContext::instances` will ever return a nullptr, and if so that should probably be addressed.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:133
> +            if (device && existsInCurrentPage(device->scriptExecutionContext()))

Is this `device` check necessary? It seems unlikely that `WebGPUDevice::instances` will ever return a nullptr, and if so that should probably be addressed.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:636
> +    // FIXME: notify old device

Still relevant?

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:117
> +        WI.Canvas._nextContextUniqueDisplayNameNumber = 1;
> +        WI.Canvas._nextDeviceUniqueDisplayNameNumber = 1;

Nit: We could drop the `WI.` prefix for these.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:166
> +                this._uniqueDisplayNameNumber = this.constructor._nextDeviceUniqueDisplayNameNumber++;

Style: Instead of `this.constructor.` I think we can just use `Canvas.`.

> Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:81
> +    // COMPATIBILITY (iOS 13): Canvas.events.cssCanvasClientNodesChanged was renamed to Canvas.events.clientNodesChanged.
> +    cssCanvasClientNodesChanged(canvasId)

Nice!
Comment 4 Devin Rousso 2019-09-10 17:23:43 PDT
Comment on attachment 378480 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Most of the actual "work" done with Web GPU actually uses a `WebGPUDevice`.
> 
> These repeated details only make sense inside of the WebCore ChangeLog. Not JavaScriptCore / WebInspectorUI.

I personally like including them in the various ChangeLogs, as it provides an overarching context to the changes.  This way, when reading JavaScriptCore changes, you can still see what's generally going on.

>> Source/WebCore/Modules/webgpu/WebGPUDevice.h:81
>> +    ~WebGPUDevice();
> 
> virtual?

YES!!!  I think this is partially the reason for some of the test issues I was encountering :D

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:124
>> +            if (context && existsInCurrentPage(context->canvasBase().scriptExecutionContext()))
> 
> Is this `context` check necessary? It seems unlikely that `CanvasRenderingContext::instances` will ever return a nullptr, and if so that should probably be addressed.

Fair point.

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:636
>> +    // FIXME: notify old device
> 
> Still relevant?

Yes, but I actually meant to rework this and kinda forgot -.-

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:117
>> +        WI.Canvas._nextDeviceUniqueDisplayNameNumber = 1;
> 
> Nit: We could drop the `WI.` prefix for these.

I love it <3

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:166
>> +                this._uniqueDisplayNameNumber = this.constructor._nextDeviceUniqueDisplayNameNumber++;
> 
> Style: Instead of `this.constructor.` I think we can just use `Canvas.`.

Ditto
Comment 5 Devin Rousso 2019-09-10 18:09:59 PDT
Created attachment 378520 [details]
[Image] After Patch is applied

(In reply to Joseph Pecoraro from comment #3)
> r=me - How does this look in the UI? Internals seem good.
The UI is the exact same, except that "Canvas" is replaced with "Device" :P

Hovering interactions also work, since we treat the "related" `HTMLCanvasElement` (via `GPUCanvasContext`) as a "client".
Comment 6 Devin Rousso 2019-09-10 18:12:52 PDT
Created attachment 378521 [details]
Patch
Comment 7 Devin Rousso 2019-09-10 19:36:25 PDT
Created attachment 378530 [details]
Patch

Fix win build
Comment 8 WebKit Commit Bot 2019-09-10 21:34:41 PDT
Comment on attachment 378530 [details]
Patch

Clearing flags on attachment: 378530

Committed r249753: <https://trac.webkit.org/changeset/249753>
Comment 9 WebKit Commit Bot 2019-09-10 21:34:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-09-10 21:35:17 PDT
<rdar://problem/55249232>
Comment 11 Truitt Savell 2019-09-11 09:18:09 PDT
It looks like the changes in https://trac.webkit.org/changeset/249753/webkit

broke inspector/canvas/shaderProgram-add-remove-webgl.html and caused it to crash on all Mac.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcanvas%2FshaderProgram-add-remove-webgl.html

Log:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249760%20(6563)/inspector/canvas/shaderProgram-add-remove-webgl-crash-log.txt

Can this be fixed today? otherwise we will need to roll this out.
Comment 12 Truitt Savell 2019-09-11 14:10:16 PDT
Reverted r249753 for reason:

caused inspector/canvas/shaderProgram-add-remove-webgl.html to crash on all Mac platforms.

Committed r249776: <https://trac.webkit.org/changeset/249776>
Comment 13 Devin Rousso 2019-09-11 15:53:35 PDT
Created attachment 378590 [details]
Patch

The values in `WebGLProgram::instances` aren't always set, as `WebGLRenderingContextBase::~WebGLRenderingContextBase` can change it to be `nullptr`.  `WebGLRenderingContextBase::~WebGLRenderingContextBase` doesn't want to remove itself from the `WebGLProgram::instances` (as in removing the key `WebGLProgram` too) because otherwise `WebGLProgram::~WebGLProgram` may fail to remove (and we would have a `WebGLProgram` that exists that isn't recorded in the instances list, which we don't want).
Comment 14 WebKit Commit Bot 2019-09-11 16:28:44 PDT
Comment on attachment 378590 [details]
Patch

Clearing flags on attachment: 378590

Committed r249786: <https://trac.webkit.org/changeset/249786>
Comment 15 WebKit Commit Bot 2019-09-11 16:28:46 PDT
All reviewed patches have been landed.  Closing bug.