Bug 172623 – Web Inspector: Instrument 2D/WebGL canvas contexts in the backend

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

[Matt Baker]

Summary:
Instrument 2D/WebGL canvas contexts in the backend.

A basic Canvas protocol should include:

Types:
- Canvas
- ContextType
- ContextAttributes

Commands:
- getCanvases

Events:
- canvasAdded
- canvasRemoved

[Radar WebKit Bug Importer]

<rdar://problem/32415986>

[Matt Baker]

Created attachment 312135 [details]
WIP - failing tests

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

Created attachment 312365 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

Created attachment 312375 [details]
Patch

[Matt Baker]

Note: context attributes were removed to reduce the complexity of this patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

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)
...

[Matt Baker]

Created attachment 312475 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Devin Rousso]

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")`);

[Joseph Pecoraro]

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.

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

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).

[Matt Baker]

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.

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

Created attachment 312706 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Devin Rousso]

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}")`);

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Devin Rousso]

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 : "";

[Matt Baker]

Created attachment 312943 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

Created attachment 313025 [details]
Patch

[Matt Baker]

Created attachment 313041 [details]
Patch

[Devin Rousso]

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()`.

[Joseph Pecoraro]

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);

[WebKit Commit Bot]

Comment on attachment 313041 [details]
Patch

Clearing flags on attachment: 313041

Committed r218376: <http://trac.webkit.org/changeset/218376>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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

[Matt Lewis]

Reverted r218376 for reason:

The patch cause multiple Layout Test Crashes.

Committed r218395: <http://trac.webkit.org/changeset/218395>

[Joseph Pecoraro]

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.

[Matt Baker]

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.

[Devin Rousso]

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.

[Matt Baker]

Created attachment 313138 [details]
Patch

[Matt Baker]

Created attachment 313140 [details]
Patch

[WebKit Commit Bot]

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

[Matt Baker]

Created attachment 313168 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 313168 [details]
Patch for landing

Clearing flags on attachment: 313168

Committed r218440: <http://trac.webkit.org/changeset/218440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

@Matt: inspector/canvas/create-canvas-contexts.html seems flacky on the mac bot (timeout).