Summary: Instrument 2D/WebGL canvas contexts in the backend. A basic Canvas protocol should include: Types: - Canvas - ContextType - ContextAttributes Commands: - getCanvases Events: - canvasAdded - canvasRemoved
<rdar://problem/32415986>
Created attachment 312135 [details] WIP - failing tests
Attachment 312135 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:54: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4] ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:55: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4] ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:67: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4] ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:68: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4] ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:69: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4] Total errors found: 5 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 312135 [details] WIP - failing tests Attachment 312135 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3884423 New failing tests: inspector/dom/highlight-shape-outside-margin.html inspector/canvas/create-canvas-contexts.html
Created attachment 312140 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312135 [details] WIP - failing tests Attachment 312135 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3884454 New failing tests: inspector/dom/highlightSelector.html inspector/dom/highlightQuad.html inspector/canvas/create-canvas-contexts.html
Created attachment 312142 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 312135 [details] WIP - failing tests Attachment 312135 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3884432 New failing tests: inspector/dom/highlight-shape-outside-margin.html inspector/dom/highlightSelector.html inspector/dom/highlightQuad.html inspector/canvas/create-canvas-contexts.html inspector/dom/highlightRect.html
Created attachment 312143 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312365 [details] Patch
Comment on attachment 312365 [details] Patch Attachment 312365 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3897053 New failing tests: inspector/canvas/create-canvas-contexts.html
Created attachment 312370 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312365 [details] Patch Attachment 312365 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3897057 New failing tests: inspector/dom/highlightRect.html
Created attachment 312372 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312375 [details] Patch
Note: context attributes were removed to reduce the complexity of this patch
Comment on attachment 312375 [details] Patch Attachment 312375 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3897634 New failing tests: inspector/dom/highlight-shape-outside-margin.html inspector/canvas/create-canvas-contexts.html inspector/dom/highlightRect.html
Created attachment 312376 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312375 [details] Patch Attachment 312375 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3897637 New failing tests: inspector/dom/highlightSelector.html
Created attachment 312378 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 312375 [details] Patch Attachment 312375 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3897653 New failing tests: inspector/dom/highlight-shape-outside-margin.html inspector/dom/highlightQuad.html
Created attachment 312380 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Fixing. These are not false positives: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x000000011251a1ee WebCore::HTMLCanvasElement::~HTMLCanvasElement() + 222 (HTMLCanvasElement.cpp:138) 1 com.apple.WebCore 0x000000011251a995 WebCore::HTMLCanvasElement::~HTMLCanvasElement() + 21 (HTMLCanvasElement.cpp:143) 2 com.apple.WebCore 0x000000011251a9b9 WebCore::HTMLCanvasElement::~HTMLCanvasElement() + 25 (HTMLCanvasElement.cpp:136) 3 com.apple.WebCore 0x0000000111be7ea8 WebCore::removeDetachedChildrenInContainer(WebCore::ContainerNode&) + 264 (ContainerNodeAlgorithms.cpp:202) 4 com.apple.WebCore 0x0000000111bd4c4e WebCore::ContainerNode::removeDetachedChildren() + 110 (ContainerNode.cpp:105) 5 com.apple.WebCore 0x0000000111bd5620 WebCore::ContainerNode::~ContainerNode() + 96 (ContainerNode.cpp:163) ...
Created attachment 312475 [details] Patch
Comment on attachment 312475 [details] Patch Attachment 312475 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3902356 New failing tests: inspector/dom/highlight-shape-outside-margin.html inspector/dom/highlightQuad.html
Created attachment 312489 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312475 [details] Patch Overall, this looks good to me. Not really sure why the tests keep failing, but I am also kinda new to working with the inspector backend. View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review > Source/WebCore/inspector/InspectorInstrumentation.cpp:1224 > +InstrumentingAgents* InspectorInstrumentation::instrumentingAgentsForRenderingContext(WebGLRenderingContextBase* context) > +{ > + if (!context) > + return nullptr; > + > + HTMLCanvasElement& canvasElement = context->canvas(); > + return instrumentingAgentsForDocument(canvasElement.document()); > +} > + Is this ever used? Is it necessary for something that I am unaware of? > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:56 > + canvasesForFrame(frame) > + { > + let canvases = []; > + for (let canvas of this._canvasIdentifierMap.values()) { > + if (canvas.parentFrame.id === frame.id) > + canvases.push(canvas); > + } > + > + return canvases; > + } FYI I removed this in my followup patches, but I think it's fine to leave for now. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:119 > +WebInspector.Canvas.ContextType = { > + Canvas2D: "canvas-context-type-canvas-2d", > + WebGL: "canvas-context-type-webgl", > +}; I think that adding the prefix to these values is unnecessary. WebInspector.Canvas.ContextType = { Canvas2D: "canvas-2d", WebGL: "webgl", }; > LayoutTests/inspector/canvas/create-canvas-contexts.html:6 > +let contexts = []; I personally like having values that should always be accessible be attached to window. I can go either way with this, though. window.contexts = []; > LayoutTests/inspector/canvas/create-canvas-contexts.html:35 > + if (window.GCController) I think you can get rid of the if check here, as the test might function differently without GCController. I'd rather get an error saying "GCController.collect is undefined" instead of wondering why the test is failing due to a lack of timely GC. > LayoutTests/inspector/canvas/create-canvas-contexts.html:84 > + resolve(); NIT: you can move this to be another `.then` .then(resolve, reject); > LayoutTests/inspector/canvas/create-canvas-contexts.html:93 > + InspectorTest.evaluateInPage("createCanvas('2d')"); I think that using template strings makes reading these easier, especially when using evaluateInPage. InspectorTest.evaluateInPage(`createCanvas("2d")`);
Comment on attachment 312475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review Nice tests! > Source/JavaScriptCore/inspector/protocol/Canvas.json:24 > + { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." }, The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary. > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:55 > +#include "InspectorInstrumentation.h" Unused. In fact all of the WebGLRenderingContextBase includes also seem unused. Seems this was intended for a later change? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:70 > + if (!m_frontendDispatcher) > + return; We shouldn't need to do this check (everywhere in this file). FrontendDispatcher will always be alive in our unique_ptr. > Source/WebCore/inspector/InspectorCanvasAgent.h:34 > +#include <wtf/HashSet.h> Unused. > Source/WebCore/inspector/InspectorCanvasAgent.h:45 > +typedef String ErrorString; This shouldn't be needed here, an include should handle it. > Source/WebCore/inspector/InspectorCanvasAgent.h:66 > + // CanvasObserver implementation We can drop the "implementation" here. > Source/WebCore/inspector/InspectorInstrumentation.cpp:996 > +void InspectorInstrumentation::didCreateCSSCanvasImpl(InstrumentingAgents* instrumentingAgents, HTMLCanvasElement& canvasElement, const String& name) Style: Lets put this above InspectorInstrumentation::networkStateChangedImpl > Source/WebCore/inspector/InspectorInstrumentation.cpp:1216 > +InstrumentingAgents* InspectorInstrumentation::instrumentingAgentsForRenderingContext(WebGLRenderingContextBase* context) Unused in this patch. > Source/WebCore/inspector/InspectorInstrumentation.h:248 > + static void didCreateCSSCanvas(HTMLCanvasElement&, const String&); Style: Lets try to keep the order of functions consistent. Here these are above "networkStateChanged" so lets keep that order. > Source/WebCore/inspector/InspectorInstrumentation.h:423 > + static void didCreateCSSCanvasImpl(InstrumentingAgents*, HTMLCanvasElement&, const String&); Style: Move these above networkStateChangedImpl > Source/WebCore/inspector/InspectorInstrumentation.h:1097 > +inline void InspectorInstrumentation::didCreateCSSCanvas(HTMLCanvasElement& canvasElement, const String& name) Style: Move these above InspectorInstrumentation::networkStateChanged > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:51 > + if (canvas.parentFrame.id === frame.id) Nit: Can we just compare `canvas.parentFrame === frame` instead of getting the ids? > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:102 > + CanvasWasRemoved: "canvas-manager-canvas-was-removed" Style: Include the trailing comma to make future changes nicer. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:50 > + case CanvasAgent.ContextType.Canvas2D: Style: dedent cases. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:67 > + case WebInspector.Canvas.ContextType.Canvas2D: Style: dedent cases. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:119 > +WebInspector.Canvas.ContextType = { > + Canvas2D: "canvas-context-type-canvas-2d", > + WebGL: "canvas-context-type-webgl", > +}; Style: Might consider making these Symbols or simpler names. I've been moving these enum types (when not events) to simple strings, like just "canvas-2d" and "webgl". It makes it much easier when debugging. > LayoutTests/inspector/canvas/create-canvas-contexts-expected.txt:9 > +Canvas added. The others all say "Added canvas" this one feels weird. > LayoutTests/inspector/canvas/create-canvas-contexts.html:9 > +if (window.testRunner) > + testRunner.overridePreference("WebKitWebGLEnabled", "1"); I do not think this is necessary. It looks like all ports default this as enabled. > LayoutTests/inspector/canvas/create-canvas-contexts.html:158 > + if (!canvas) { > + reject(); > + return; > + } Doesn't look like this can happen. Probably would have gone to the catch path anyways.
Comment on attachment 312475 [details] Patch Attachment 312475 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3902788 New failing tests: inspector/dom/highlightSelector.html inspector/dom/highlightFrame.html inspector/canvas/create-canvas-contexts.html
Created attachment 312500 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review >> Source/JavaScriptCore/inspector/protocol/Canvas.json:24 >> + { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." }, > > The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary. Will remove the overload. The element ID was only used for building a display name in the frontend (Canvas #my-canvas"). Adding a DOM.NodeId property to Canvas would allow it to be retrieved in the frontend. >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:55 >> +#include "InspectorInstrumentation.h" > > Unused. In fact all of the WebGLRenderingContextBase includes also seem unused. Seems this was intended for a later change? Removed, but I'm not sure what "all of the WebGLRenderingContextBase" includes means. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:70 >> + return; > > We shouldn't need to do this check (everywhere in this file). FrontendDispatcher will always be alive in our unique_ptr. It looks like InspectorDOMAgent::focusNode() is the only other place making this check. Is it also unnecessary? >> Source/WebCore/inspector/InspectorCanvasAgent.h:45 >> +typedef String ErrorString; > > This shouldn't be needed here, an include should handle it. I don't think ErrorString is included from anywhere. Every agent defines ErrorString this way in it's header. >> Source/WebCore/inspector/InspectorInstrumentation.cpp:1224 >> + > > Is this ever used? Is it necessary for something that I am unaware of? Leftover from before splitting up the patch. Good eye. >> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:51 >> + if (canvas.parentFrame.id === frame.id) > > Nit: Can we just compare `canvas.parentFrame === frame` instead of getting the ids? Yes, but I'm going to remove this per Devin's comment. >>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:119 >>> +}; >> >> I think that adding the prefix to these values is unnecessary. >> >> WebInspector.Canvas.ContextType = { >> Canvas2D: "canvas-2d", >> WebGL: "webgl", >> }; > > Style: Might consider making these Symbols or simpler names. I've been moving these enum types (when not events) to simple strings, like just "canvas-2d" and "webgl". It makes it much easier when debugging. I like shortening them. I'm (irrationally) paranoid about collisions with other enums, so I'll go with Symbols. >> LayoutTests/inspector/canvas/create-canvas-contexts.html:9 >> + testRunner.overridePreference("WebKitWebGLEnabled", "1"); > > I do not think this is necessary. It looks like all ports default this as enabled. Nice. >> LayoutTests/inspector/canvas/create-canvas-contexts.html:93 >> + InspectorTest.evaluateInPage("createCanvas('2d')"); > > I think that using template strings makes reading these easier, especially when using evaluateInPage. > > InspectorTest.evaluateInPage(`createCanvas("2d")`); Joe has been doing this too, since it makes adding template literals easier in the future. Will adopt the pattern (at least for all evaluateInPage).
Comment on attachment 312475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review >>> Source/JavaScriptCore/inspector/protocol/Canvas.json:24 >>> + { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." }, >> >> The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary. > > Will remove the overload. The element ID was only used for building a display name in the frontend (Canvas #my-canvas"). Adding a DOM.NodeId property to Canvas would allow it to be retrieved in the frontend. Also removing `isCSSCanvas`. Replacing with an optional property `cssCanvasName`. Adding optional property `nodeId` as well.
Comment on attachment 312475 [details] Patch Attachment 312475 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3903674 New failing tests: inspector/dom/highlight-shape-outside-margin.html inspector/dom/highlightQuad.html
Created attachment 312519 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 312706 [details] Patch
Comment on attachment 312706 [details] Patch Attachment 312706 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3919663 Number of test failures exceeded the failure limit.
Created attachment 312714 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312706 [details] Patch r=me with the one issue on InspectorCanvasAgent.cpp:222. View in context: https://bugs.webkit.org/attachment.cgi?id=312706&action=review > Source/WebCore/html/HTMLCanvasElement.cpp:251 > + InspectorInstrumentation::didCreateCanvasRenderingContext(*this); NIT: Does this need to be called after `invalidateStyleAndLayerComposition()`? I am not sure of the process here, but it seems like that is an important step (it always occurs for WebGL contexts). > Source/WebCore/inspector/InspectorCanvasAgent.cpp:222 > + if (!nodeId) { > + ErrorString ignored; > + nodeId = domAgent->pushNodeToFrontend(ignored, domAgent->boundNodeId(&canvasElement.document()), &canvasElement); > + } > + canvas->setNodeId(nodeId); I think there is an edge-case that might cause this code to behave badly. When I was testing, I found that calling InspectorCanvasAgent::enable on a page with active <canvas> contexts would cause this code to run before the main <document> is sent to the frontend (via `WebInspector.domTreeManager.requestDocument`, which calls `DOMAgent.getDocument`). Since "nodeId" is already optional, I think we shouldn't even set a value in the case that the <document> hasn't already been sent to the frontend. int nodeId = domAgent->boundNodeId(&canvasElement); if (!nodeId) { if (int documentNodeId = domAgent->boundNodeId(&canvasElement.document())) { ErrorString ignored; nodeId = domAgent->pushNodeToFrontend(ignored, documentNodeId, &canvasElement); } } if (nodeId) canvas->setNodeId(nodeId); > Source/WebCore/inspector/InspectorInstrumentation.h:1095 > + Style: remove spaces. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:34 > + console.assert(frame); Is there a reason to not assert that the frame is a WebInspector.Frame? console.assert(frame instanceof WebInspector.Frame); > LayoutTests/inspector/canvas/create-canvas-contexts.html:86 > + InspectorTest.evaluateInPage(`createCanvas('2d')`); Style: should use " instead of ' InspectorTest.evaluateInPage(`createCanvas("2d")`); > LayoutTests/inspector/canvas/create-canvas-contexts.html:117 > + expression: "createCanvas('2d')", Ditto. expression: `createCanvas("2d")`, > LayoutTests/inspector/canvas/create-canvas-contexts.html:122 > + expression: "createCanvas('webgl')", Ditto. expression: `createCanvas("webgl")`, > LayoutTests/inspector/canvas/create-canvas-contexts.html:127 > + expression: "createOffscreenCanvas('2d')", Ditto. expression: `createOffscreenCanvas("2d")`, > LayoutTests/inspector/canvas/create-canvas-contexts.html:132 > + expression: "createOffscreenCanvas('webgl')", Ditto. expression: `createOffscreenCanvas("webgl")`, > LayoutTests/inspector/canvas/create-canvas-contexts.html:155 > + InspectorTest.evaluateInPage(`createCSSCanvas('${contextId}', '${cssCanvasIdentifier}')`); Ditto. InspectorTest.evaluateInPage(`createCSSCanvas("${contextId}", "${cssCanvasIdentifier}")`);
Comment on attachment 312706 [details] Patch Attachment 312706 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3919765 Number of test failures exceeded the failure limit.
Created attachment 312718 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 312706 [details] Patch Attachment 312706 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3919679 Number of test failures exceeded the failure limit.
Created attachment 312719 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312706&action=review > Source/WebInspectorUI/UserInterface/Models/Canvas.js:110 > + cookie[WebInspector.Canvas.NodePathCookieKey] = this._domNode ? this._domNode.path || ""; This causes an error. It should be ":", not "||". cookie[WebInspector.Canvas.NodePathCookieKey] = this._domNode ? this._domNode.path : "";
Created attachment 312943 [details] Patch
Comment on attachment 312943 [details] Patch Attachment 312943 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3932764 New failing tests: inspector/canvas/create-canvas-contexts.html
Created attachment 312945 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 313025 [details] Patch
Created attachment 313041 [details] Patch
Comment on attachment 313041 [details] Patch r=me View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review > Source/WebCore/inspector/InspectorCanvasAgent.cpp:111 > + if (!m_enabled) { > + m_canvasEntries.clear(); > + return; > + } > + > + for (auto* canvasElement : canvasesForFrame) { > + auto canvasEntry = m_canvasEntries.take(canvasElement); > + m_frontendDispatcher->canvasRemoved(canvasEntry.identifier); > + } I don't think we want to do this. We should only be clearing the canvases for the frame that navigated: for (auto* canvasElement : canvasesForFrame) { auto canvasEntry = m_canvasEntries.take(canvasElement); if (m_enabled) m_frontendDispatcher->canvasRemoved(canvasEntry.identifier); } > Source/WebCore/inspector/InspectorCanvasAgent.cpp:132 > + newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.get(&canvasElement); > + m_canvasToCSSCanvasId.remove(&canvasElement); I think you can just use `take` here like above: if (m_canvasToCSSCanvasId.contains(&canvasElement)) newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement); > Source/WebCore/inspector/InspectorCanvasAgent.cpp:173 > + m_frontendDispatcher->canvasRemoved(identifier); I think that it might be possible for this to run even after `disable()` has been called. I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`.
Comment on attachment 313041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review > Source/WebCore/inspector/InspectorCanvasAgent.cpp:90 > +void InspectorCanvasAgent::frameNavigated(DocumentLoader* loader) Why doesn't this just take a Frame& / Frame*? This only uses the DocumentLoader to access the Frame. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132 >> + m_canvasToCSSCanvasId.remove(&canvasElement); > > I think you can just use `take` here like above: > > if (m_canvasToCSSCanvasId.contains(&canvasElement)) > newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement); Looks like this could just be: newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement);
Comment on attachment 313041 [details] Patch Clearing flags on attachment: 313041 Committed r218376: <http://trac.webkit.org/changeset/218376>
All reviewed patches have been landed. Closing bug.
After this patch, many layout tests involving canvas were crashing and failing. https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/1578 https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20%28Tests%29/builds/1578/steps/layout-test/logs/stdio
Reverted r218376 for reason: The patch cause multiple Layout Test Crashes. Committed r218395: <http://trac.webkit.org/changeset/218395>
Comment on attachment 313041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review > Source/WebCore/inspector/InspectorCanvasAgent.cpp:150 > + canvasElement.removeObserver(*this); HTMLCanvasElement triggers canvasDestroyed in its destructor while iterating its observers: for (auto& observer : m_observers) observer->canvasDestroyed(*this); If CanvasAgent::canvasDestroyed then tries to remove itself as an observer here then we will be mutating the HashMap that we are iterating over. I suspect this is what the ASSERT is catching and causing crashes.
Comment on attachment 313041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:90 >> +void InspectorCanvasAgent::frameNavigated(DocumentLoader* loader) > > Why doesn't this just take a Frame& / Frame*? This only uses the DocumentLoader to access the Frame. I think this was left over from an older implementation. Other agents that define frameNavigated take a Frame&. Will fix. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:150 >> + canvasElement.removeObserver(*this); > > HTMLCanvasElement triggers canvasDestroyed in its destructor while iterating its observers: > > for (auto& observer : m_observers) > observer->canvasDestroyed(*this); > > If CanvasAgent::canvasDestroyed then tries to remove itself as an observer here then we will be mutating the HashMap that we are iterating over. > > I suspect this is what the ASSERT is catching and causing crashes. It's safe for CanvasObservers to assume that the observer is removed as a side effect of canvasDestroyed. Will the call to removeObserver in InspectorCanvasAgent::canvasDestroyed. >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173 >> + m_frontendDispatcher->canvasRemoved(identifier); > > I think that it might be possible for this to run even after `disable()` has been called. I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`. Good catch. Stopping the timer in `disable()` feels cleaner. Will fix.
Comment on attachment 313041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review >>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173 >>> + m_frontendDispatcher->canvasRemoved(identifier); >> >> I think that it might be possible for this to run even after `disable()` has been called. I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`. > > Good catch. Stopping the timer in `disable()` feels cleaner. Will fix. If you go that route, you're also going to have to make sure to call `m_removedCanvasIdentifiers.clear();` so that if the agent is enabled again later on there aren't any leftover removed canvases that dispatch events.
Created attachment 313138 [details] Patch
Created attachment 313140 [details] Patch
Comment on attachment 313140 [details] Patch Rejecting attachment 313140 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 313140, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: LayoutTests :040000 040000 00c58440e2558cd462d45456051fbb940ff4883b df50ec9c69de9742bac5f1e63c6813cd5d3d421b M Source Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-queues.webkit.org/results/3944929
Created attachment 313168 [details] Patch for landing
Comment on attachment 313168 [details] Patch for landing Clearing flags on attachment: 313168 Committed r218440: <http://trac.webkit.org/changeset/218440>
@Matt: inspector/canvas/create-canvas-contexts.html seems flacky on the mac bot (timeout).