Summary: | Web Inspector: Canvas: lazily create the agent | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, darin, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, hi, inspector-bugzilla-changes, joepeck, kondapallykalyan, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 195825 | ||||||||||||||
Attachments: |
|
Description
Devin Rousso
2019-03-01 21:28:28 PST
Created attachment 363410 [details]
Patch
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 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. 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
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. Correction: in the future not all *targets* will have this agent. 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. Created attachment 363481 [details] Patch Addressed Joe's responses from comment #6. Still need to do as I'd said in comment #8. 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 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? Created attachment 363531 [details]
Patch
Created attachment 363549 [details]
Patch
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 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 on attachment 363549 [details] Patch Clearing flags on attachment: 363549 Committed r242594: <https://trac.webkit.org/changeset/242594> All reviewed patches have been landed. Closing bug. |