Bug 180770 – Web Inspector: replace HTMLCanvasElement with CanvasRenderingContext for instrumentation logic

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

[Devin Rousso]

Since everything we do with canvases really only cares about the CanvasRenderingContext (except for the DOM Node), we should really be tracking those instead of instances of HTMLCanvasElement.  

This should also make it smoother to provide support for OffscreenCanvas once it's complete.

[Devin Rousso]

Created attachment 329265 [details]
[Patch] WIP

[Joseph Pecoraro]

Comment on attachment 329265 [details]
[Patch] WIP

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

> Source/JavaScriptCore/inspector/protocol/Canvas.json:-49
> -                { "name": "frameId", "$ref": "Network.FrameId", "description": "Parent frame identifier." },

If the frontend wants to associate the Canvas with a Frame it would need to use resolve this to a DOMNode and then get the node's frame? That seems okay.

> Source/WebCore/inspector/InspectorCanvas.cpp:95
> +HTMLCanvasElement* InspectorCanvas::getIfHTMLCanvasElement()

This is a clunky name. How about:

    HTMLCanvasElement* canvasElement() const;

It seems clear this would return a nullptr if this is not an HTMLCanvasElement.

[Devin Rousso]

Comment on attachment 329265 [details]
[Patch] WIP

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

>> Source/JavaScriptCore/inspector/protocol/Canvas.json:-49
>> -                { "name": "frameId", "$ref": "Network.FrameId", "description": "Parent frame identifier." },
> 
> If the frontend wants to associate the Canvas with a Frame it would need to use resolve this to a DOMNode and then get the node's frame? That seems okay.

Yeah.  I was trying to think about it, but when would we want to do that?  I figure that it might be better for us to provide data relating to what context/target (document vs worker) we are dealing with, but that would be for a followup (and once worker support is done).

>> Source/WebCore/inspector/InspectorCanvas.cpp:95
>> +HTMLCanvasElement* InspectorCanvas::getIfHTMLCanvasElement()
> 
> This is a clunky name. How about:
> 
>     HTMLCanvasElement* canvasElement() const;
> 
> It seems clear this would return a nullptr if this is not an HTMLCanvasElement.

I personally don't like that naming convention, as just having an HTMLCanvasElement* doesn't mean that it might be a nullptr.  Also, it would make naming a bit more annoying at a few of the call-sites (e.g. InspectorCanvas::buildObjectForCanvas) where there is already a `canvas` or `element` variable, especially since I can't reuse `canvasElement`.  If you have a better name that avoids this, I'd be happy to change it.

[EWS Watchlist]

Comment on attachment 329265 [details]
[Patch] WIP

Attachment 329265 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5650968

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
inspector/canvas/create-context-webgl.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329279 [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

[Joseph Pecoraro]

Comment on attachment 329265 [details]
[Patch] WIP

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

>>> Source/JavaScriptCore/inspector/protocol/Canvas.json:-49
>>> -                { "name": "frameId", "$ref": "Network.FrameId", "description": "Parent frame identifier." },
>> 
>> If the frontend wants to associate the Canvas with a Frame it would need to use resolve this to a DOMNode and then get the node's frame? That seems okay.
> 
> Yeah.  I was trying to think about it, but when would we want to do that?  I figure that it might be better for us to provide data relating to what context/target (document vs worker) we are dealing with, but that would be for a followup (and once worker support is done).

I'd guess that an OffscreenCanvas created by a Worker would be exposed through the Worker's InspectorController (all happening on the Worker's Thread).

In which case we'd:
• Enable the Canvas domain for Workers
• Give WI.Canvas a `target` property and populate it.

At which point we'd be able to know from `canvas.target` if it was in a Page / Worker.

[EWS Watchlist]

Comment on attachment 329265 [details]
[Patch] WIP

Attachment 329265 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5650987

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
inspector/canvas/create-context-webgl.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329282 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329265 [details]
[Patch] WIP

Attachment 329265 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5651127

New failing tests:
fast/canvas/webgl/context-release-upon-reload.html
webgl/1.0.2/conformance/context/context-release-with-workers.html

[EWS Watchlist]

Created attachment 329287 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 329265 [details]
[Patch] WIP

Attachment 329265 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5651926

New failing tests:
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 329294 [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

[Devin Rousso]

Created attachment 329306 [details]
[Patch] WIP

Rebase

[EWS Watchlist]

Comment on attachment 329306 [details]
[Patch] WIP

Attachment 329306 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5654045

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329315 [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

[EWS Watchlist]

Comment on attachment 329306 [details]
[Patch] WIP

Attachment 329306 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5654238

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 329320 [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

[EWS Watchlist]

Comment on attachment 329306 [details]
[Patch] WIP

Attachment 329306 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5654261

New failing tests:
fast/canvas/webgl/context-release-upon-reload.html
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329323 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 329324 [details]
Patch

[EWS Watchlist]

Comment on attachment 329324 [details]
Patch

Attachment 329324 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5655226

New failing tests:
fast/canvas/webgl/context-release-upon-reload.html
webgl/1.0.2/conformance/context/context-release-with-workers.html

[EWS Watchlist]

Created attachment 329326 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 329324 [details]
Patch

Attachment 329324 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5655412

New failing tests:
fast/canvas/webgl/context-release-upon-reload.html
inspector/canvas/create-context-bitmaprenderer.html
webgl/1.0.2/conformance/context/context-release-with-workers.html
inspector/canvas/create-context-webgl.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329327 [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

[Devin Rousso]

Created attachment 329328 [details]
Patch

I think some of the test failures were due to the fact that the CanvasRenderingContext would be destroyed before the frame navigation event is fired, but I could be wrong

[EWS Watchlist]

Comment on attachment 329328 [details]
Patch

Attachment 329328 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5656046

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 329330 [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

[EWS Watchlist]

Comment on attachment 329328 [details]
Patch

Attachment 329328 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5656175

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 329334 [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]

Created attachment 329336 [details]
Patch

Based on the mac-debug results, I think we do need a timer when destroying contexts to prevent JS allocations.

[EWS Watchlist]

Comment on attachment 329336 [details]
Patch

Attachment 329336 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5656962

New failing tests:
fast/canvas/webgl/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329338 [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

[EWS Watchlist]

Comment on attachment 329336 [details]
Patch

Attachment 329336 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5656975

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329339 [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

[EWS Watchlist]

Comment on attachment 329336 [details]
Patch

Attachment 329336 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5656986

New failing tests:
fast/canvas/webgl/context-release-upon-reload.html
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329340 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 329336 [details]
Patch

Attachment 329336 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5656987

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329341 [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

[Devin Rousso]

Created attachment 329353 [details]
Patch

Oops.  I added back the logic inside frameNavigated.

[Joseph Pecoraro]

Comment on attachment 329353 [details]
Patch

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

r=me

> Source/WebCore/inspector/InspectorCanvas.cpp:96
> +HTMLCanvasElement* InspectorCanvas::getIfHTMLCanvasElement()

I still don't like this name. We normally don't include `get` in a getter name either.

> Source/WebCore/inspector/InspectorCanvas.cpp:313
> +    auto* canvasElement = getIfHTMLCanvasElement();

Should we have a FIXME here for OffscreenCanvas?

> Source/WebCore/inspector/InspectorCanvas.h:49
> +class OffscreenCanvas;

Not used?

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:153
> +        errorString = ASCIILiteral("OffscreenCanvas is currently not supported.");

OffscreenCanvas won't have a node. So probably a better error might be something like:

    errorString = ASCIILiteral("No node for this canvas");
    errorString = ASCIILiteral("No node for OffscreenCanvas");

Also I know we aren't consistent, but I don't think we have periods with these errors most of the time.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:215
> +    auto* canvasElement = inspectorCanvas->getIfHTMLCanvasElement();
> +    if (!canvasElement) {
> +        errorString = ASCIILiteral("OffscreenCanvas is currently not supported.");
> +        return;
> +    }

Would it be possible for an OffscreenCanvas to have client nodes? If not we should update this error messages.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:-380
> -    Vector<InspectorCanvas*> inspectorCanvases;
> -    for (auto& inspectorCanvas : m_identifierToInspectorCanvas.values()) {
> -        if (inspectorCanvas->canvas().document().frame() == &frame)
> -            inspectorCanvases.append(inspectorCanvas.get());
> -    }
> -
> -    for (auto* inspectorCanvas : inspectorCanvases) {
> -        String identifier = unbindCanvas(*inspectorCanvas);
> -        if (m_enabled)
> -            m_frontendDispatcher->canvasRemoved(identifier);
> -    }

Hmm. So this relies on the GC to eliminate canvases in subframes instead of immediately eliminating them when the frame goes away? I suppose this is fine but if I have a long running top frame and short lived sub-frames it might be annoying if the frontend updates out of sync with the frames coming and going.

[Devin Rousso]

Created attachment 329395 [details]
Patch

[Devin Rousso]

Created attachment 329399 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329399 [details]
Patch

Clearing flags on attachment: 329399

Committed r225941: <https://trac.webkit.org/changeset/225941>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36061461>

[Ryan Haddad]

webgl/1.0.2/conformance/context/context-release-with-workers.html is failing an assertion after this change:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r225980%20(5403)/results.html

SHOULD NEVER BE REACHED
/Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp(397) : void WebCore::InspectorCanvasAgent::didCreateCanvasRenderingContext(WebCore::CanvasRenderingContext &)
1   0x10be08dfd WTFCrash
2   0x116183bc5 WebCore::InspectorCanvasAgent::didCreateCanvasRenderingContext(WebCore::CanvasRenderingContext&)
3   0x116131395 WebCore::InspectorInstrumentation::didCreateCanvasRenderingContextImpl(WebCore::InstrumentingAgents&, WebCore::CanvasRenderingContext&)
4   0x116033bc1 WebCore::InspectorInstrumentation::didCreateCanvasRenderingContext(WebCore::CanvasRenderingContext&)
5   0x11723021e WebCore::WebGLRenderingContext::create(WebCore::CanvasBase&, WTF::Ref<WebCore::GraphicsContext3D>&&, WebCore::GraphicsContext3DAttributes)
6   0x1172400ef WebCore::WebGLRenderingContextBase::create(WebCore::CanvasBase&, WebCore::GraphicsContext3DAttributes&, WTF::String const&)
7   0x115ee752b WebCore::HTMLCanvasElement::createContextWebGL(WTF::String const&, WebCore::GraphicsContext3DAttributes&&)
8   0x115ee6503 WebCore::HTMLCanvasElement::getContext(JSC::ExecState&, WTF::String const&, WTF::Vector<JSC::Strong<JSC::Unknown>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&)
9   0x114826c8d WebCore::jsHTMLCanvasElementPrototypeFunctionGetContextBody(JSC::ExecState*, WebCore::JSHTMLCanvasElement*, JSC::ThrowScope&)
10  0x11481f70e long long WebCore::IDLOperation<WebCore::JSHTMLCanvasElement>::call<&(WebCore::jsHTMLCanvasElementPrototypeFunctionGetContextBody(JSC::ExecState*, WebCore::JSHTMLCanvasElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
11  0x11481f49c WebCore::jsHTMLCanvasElementPrototypeFunctionGetContext(JSC::ExecState*)
12  0x36c7aa401178
13  0x10a99721b llint_entry
14  0x10a99721b llint_entry
15  0x10a98f2f7 vmEntryToJavaScript
16  0x10b6c439e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
17  0x10b66a1e2 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
18  0x10b8e6ee7 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
19  0x10b8e7030 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
20  0x11578e19b WebCore::JSMainThreadExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
21  0x11577bc78 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*)
22  0x11577bd9d WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*)
23  0x115d131e7 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
24  0x115d11882 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)
25  0x116076b00 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&)
26  0x11607696f WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement>&&, WTF::TextPosition const&)
27  0x1160589e5 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
28  0x116058b33 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&)
29  0x116057c28 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
30  0x11605783b WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
31  0x116059a8a WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl>&&)

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r225980%20(5403)/results.html

[Ryan Haddad]

Reverted r225941 for reason:

This change introduced LayoutTest crashes and assertion failures.

Committed r225990: <https://trac.webkit.org/changeset/225990>

[Devin Rousso]

Created attachment 329562 [details]
Patch

I'm not 100% sure about this, but I think that the InstrumentingAgents are wiped out whenever a Frame is removed.  The failing tests checked that canvas contexts within iframes are destroyed after the iframe navigates or is removed.  As a result, since the InstrumentingAgents don't exist anymore, we don't have a way to remove the existing contexts from the CanvasAgent's list, meaning that they are still listed there even though they don't exist anymore.  The solution is to not delete the code inside InspectorCanvasAgent::frameNavigated that finds all the tracked contexts for the navigating frame and removes them.  Hopefully :|

[EWS Watchlist]

Comment on attachment 329562 [details]
Patch

Attachment 329562 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5681941

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329564 [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

[EWS Watchlist]

Comment on attachment 329562 [details]
Patch

Attachment 329562 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5681960

New failing tests:
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329565 [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

[EWS Watchlist]

Comment on attachment 329562 [details]
Patch

Attachment 329562 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5681966

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329567 [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

[EWS Watchlist]

Comment on attachment 329562 [details]
Patch

Attachment 329562 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5681970

New failing tests:
fast/canvas/webgl/context-release-upon-reload.html
webgl/1.0.2/conformance/context/context-release-with-workers.html
webgl/1.0.2/conformance/context/context-release-upon-reload.html

[EWS Watchlist]

Created attachment 329569 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 329588 [details]
Patch

Taking a different approach.  Instead of trying to add InspectorInstrumentation hooks to ~CanvasRenderingContext, we can expand the usage of the CanvasObserver concept and rely on the destruction of the CanvasBase to know when the CanvasRenderingContext will be destroyed.

[EWS Watchlist]

Comment on attachment 329588 [details]
Patch

Attachment 329588 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5689923

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329589 [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

[EWS Watchlist]

Comment on attachment 329588 [details]
Patch

Attachment 329588 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5690175

New failing tests:
inspector/canvas/requestContent-webgl.html
inspector/dom/highlightQuad.html

[EWS Watchlist]

Created attachment 329590 [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

[EWS Watchlist]

Comment on attachment 329588 [details]
Patch

Attachment 329588 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5690242

New failing tests:
inspector/dom/highlightRect.html

[EWS Watchlist]

Created attachment 329591 [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

[Devin Rousso]

Created attachment 329594 [details]
Patch

[EWS Watchlist]

Comment on attachment 329594 [details]
Patch

Attachment 329594 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5692347

New failing tests:
inspector/dom/highlightSelector.html
inspector/dom/highlightQuad.html

[EWS Watchlist]

Created attachment 329596 [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

[Devin Rousso]

Created attachment 329597 [details]
Patch

I love it how all of these tests run fine on my machine...

[EWS Watchlist]

Comment on attachment 329597 [details]
Patch

Attachment 329597 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5693325

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329598 [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

[Devin Rousso]

Created attachment 329599 [details]
Patch

Rebase

[EWS Watchlist]

Comment on attachment 329599 [details]
Patch

Attachment 329599 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5694498

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html

[EWS Watchlist]

Created attachment 329600 [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

[Devin Rousso]

Created attachment 329619 [details]
Patch

It looks like it's possible for willDeleteProgram and isShaderProgramDisabled to be called on a ShaderProgram that has already been deleted via deleteProgram, so we don't want to ASSERT for an InspectorShaderProgram.

[Devin Rousso]

Created attachment 329686 [details]
Patch

Let's see if I can get the win EWS to be happy :P

[Joseph Pecoraro]

Comment on attachment 329686 [details]
Patch

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

> Source/WebCore/ChangeLog:138
> [phone]  Megan Gardner  <[e-mail]>

Extra 2.

> Source/WebCore/ChangeLog:-258
> -2017-12-16  Dan Bernstein  <[e-mail]>

Missing 2.

[Devin Rousso]

Created attachment 329709 [details]
Patch

Oops.

[EWS Watchlist]

Comment on attachment 329709 [details]
Patch

Attachment 329709 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5736444

New failing tests:
fast/mediastream/MediaStream-MediaElement-setObject-null.html

[EWS Watchlist]

Created attachment 329720 [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

[Devin Rousso]

Created attachment 329729 [details]
Patch

Rebase

[Joseph Pecoraro]

Comment on attachment 329729 [details]
Patch

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

Some open questions.

> Source/WebCore/html/CanvasBase.h:29
> +#include "IntSize.h"

Is this needed? It is also forward declared below.

> Source/WebCore/html/CanvasBase.h:82
> +    HashSet<CanvasObserver*> m_observers;

I'd have expected this to be private.

> Source/WebCore/html/OffscreenCanvas.cpp:45
> +OffscreenCanvas::~OffscreenCanvas()

Why isn't this done in ~CanvasBase?

> Tools/ChangeLog:8
> +        * Scripts/webkitpy/common/config/watchlist:

The watchlist stuff could probably be its own unrelated change. No need to include it with a code change like this. Its also super generic given there are 260+ files many (WebGPU).

[Devin Rousso]

Comment on attachment 329729 [details]
Patch

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

>> Source/WebCore/html/OffscreenCanvas.cpp:45
>> +OffscreenCanvas::~OffscreenCanvas()
> 
> Why isn't this done in ~CanvasBase?

This isn't done in ~CanvasBase because of what ~HTMLCanvasElement does, specifically that we manually set `m_context = nullptr;`.  Due to the inheritance destruction order, having the `CanvasObserver::canvasDestroyed` call be in `~CanvasBase` would mean that the `m_context` would be `nullptr` by the time of the `CanvasObserver::canvasDestroyed` call.  As a result, we keep the existing functionality of `~HTMLCanvasElement`.

If we added the `CanvasObserver::canvasDestroyed` calls to `~CanvasBase`, each `CanvasObserver::canvasDestroyed` would get called multiple times for the same canvas.  Instead, we don't add it to the base and have the subclasses manage the observers themselves (this is also why `m_observers` is protected).

[Joseph Pecoraro]

Comment on attachment 329729 [details]
Patch

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

>>> Source/WebCore/html/OffscreenCanvas.cpp:45
>>> +OffscreenCanvas::~OffscreenCanvas()
>> 
>> Why isn't this done in ~CanvasBase?
> 
> This isn't done in ~CanvasBase because of what ~HTMLCanvasElement does, specifically that we manually set `m_context = nullptr;`.  Due to the inheritance destruction order, having the `CanvasObserver::canvasDestroyed` call be in `~CanvasBase` would mean that the `m_context` would be `nullptr` by the time of the `CanvasObserver::canvasDestroyed` call.  As a result, we keep the existing functionality of `~HTMLCanvasElement`.
> 
> If we added the `CanvasObserver::canvasDestroyed` calls to `~CanvasBase`, each `CanvasObserver::canvasDestroyed` would get called multiple times for the same canvas.  Instead, we don't add it to the base and have the subclasses manage the observers themselves (this is also why `m_observers` is protected).

Okay, then I think we really should make this less prone to error that someone subclasses CanvasBase and forgets to notify observers.

Worst case something like:

    class CanvasBase {
    ...
    protected:
        // Subclasses are expected to notify observers in destruction.
        HashSet<CanvasObserver*> m_observers;
        void notifyObserversOfDestruction();
    #ifndef NDEBUG
        bool m_didNotifyObserversOfDestruction { false };
    #endif
    ...
    }

With implementation:

    CanvasBase::~CanvasBase()
    {
        ASSERT(m_didNotifyObserversOfDestruction);
    }

    void CanvasBase::notifyObserversOfDestruction()
    {
#ifndef NDEBUG
        ASSERT(!m_didNotifyObserversOfDestruction);
        m_didNotifyObserversOfDestruction = true;
#endif
        for (auto& observer : m_observers)
            observer->canvasDestroyed(*this);
    }

Would mean when the next person adds `class FutureCanvas : public <CanvasBase>` we would immediately catch if they didn't handle destruction.

---

But all of that said, it would be best to just make this work as expected and make m_observers managed only by CanvasBase.

[Devin Rousso]

Created attachment 330506 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 330506 [details]
Patch

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

r=me

> Source/WebCore/html/CanvasBase.h:63
> +    void addObserver(CanvasObserver&);
> +    void removeObserver(CanvasObserver&);

Style: I'd put these after the list of virtual methods instead of in the middle. And maybe the notifyObserver functions could be made protected instead of public, but public is fine.

[Devin Rousso]

Created attachment 330519 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 330519 [details]
Patch

Clearing flags on attachment: 330519

Committed r226439: <https://trac.webkit.org/changeset/226439>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.