Bug 196082 – Web Inspector: Safari Canvas Inspector seems to show the canvas being rendered twice per frame.

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

Created attachment 365560 [details]
Capture of the canvas inspector

I put together a very simple example displaying just an image and run it through Safari Canvas Inspector and the Inspector shows drawElements and clear methods being called twice per frame.

https://j3o32qpxpy.codesandbox.io/

This also happens when inspecting another very simple example (native WebGL this time): https://www.khronos.org/registry/webgl/sdk/demos/webkit/SpinningBox.html

So the conclusion is that Safari somehow miscalculates when a frame starts or ends.

See https://github.com/mrdoob/three.js/issues/16028 for related discussion

[Radar WebKit Bug Importer]

<rdar://problem/49113496>

[Devin Rousso]

Created attachment 365658 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 365658 [details]
Patch

Clearing flags on attachment: 365658

Committed r243356: <https://trac.webkit.org/changeset/243356>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 365658 [details]
Patch

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

> Source/WebCore/inspector/InspectorCanvas.cpp:386
> +    m_bufferUsed -= m_actionNeedingSnapshot->memoryCost();
> +
> +    ErrorString ignored;
> +    m_actionNeedingSnapshot->addItem(indexForData(getCanvasContentAsDataURL(ignored)));
> +
> +    m_bufferUsed += m_actionNeedingSnapshot->memoryCost();

This adjusting with memoryCost() is new. `m_bufferUsed` is an unsigned type `size_t` should we be worried about potential underflowing/overflowing or is the bufferUsed guaranteed to already contain at least action needing snapshot? It seems this arithmetic is happening to allow this to happen at no cost?

[Devin Rousso]

Comment on attachment 365658 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvas.cpp:386
>> +    m_bufferUsed += m_actionNeedingSnapshot->memoryCost();
> 
> This adjusting with memoryCost() is new. `m_bufferUsed` is an unsigned type `size_t` should we be worried about potential underflowing/overflowing or is the bufferUsed guaranteed to already contain at least action needing snapshot? It seems this arithmetic is happening to allow this to happen at no cost?

I don't think we realistically need to worry about overflow.  In order for that to happen, given that `m_actionNeedingSnapshot` is a serialized JSON value, we'd need to have a JSON file that's over 4GB in size.  At that point, Web Inspector itself would run out of memory.  Yes, we could impose some sort of limit, but there's realistically no single action that can be performed by a canvas (right now) that would result in such a massive payload (yes, someone could try to `drawImage` with some <img> that's massive, but at that point I'd imagine that there would be bigger problems).  Regardless, we default the buffer limit to 100MB (unless otherwise set by `console.record`), so we should be safe.

TL;DR: no, we shouldn't have to worry about this because that would be such a massive amount of data that it's effectively outside the realm of possibility.

[Ryan Haddad]

This change caused webgl layout tests to crash with an assertion failure on debug bots:

ASSERTION FAILED: m_table
/Volumes/Data/slave/mojave-debug/build/WebKitBuild/Debug/usr/local/include/wtf/HashTable.h(213) : void WTF::HashTableConstIterator<WebCore::GraphicsContext3D::Client *, WebCore::GraphicsContext3D::Client *, WTF::IdentityExtractor, WTF::PtrHash<WebCore::GraphicsContext3D::Client *>, WTF::HashTraits<WebCore::GraphicsContext3D::Client *>, WTF::HashTraits<WebCore::GraphicsContext3D::Client *> >::checkValidity() const [Key = WebCore::GraphicsContext3D::Client *, Value = WebCore::GraphicsContext3D::Client *, Extractor = WTF::IdentityExtractor, HashFunctions = WTF::PtrHash<WebCore::GraphicsContext3D::Client *>, Traits = WTF::HashTraits<WebCore::GraphicsContext3D::Client *>, KeyTraits = WTF::HashTraits<WebCore::GraphicsContext3D::Client *>]
1   0x115109119 WTFCrash
2   0x11c910c6b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x11fd531b9 WTF::HashTableConstIterator<WebCore::GraphicsContext3D::Client*, WebCore::GraphicsContext3D::Client*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::GraphicsContext3D::Client*>, WTF::HashTraits<WebCore::GraphicsContext3D::Client*>, WTF::HashTraits<WebCore::GraphicsContext3D::Client*> >::checkValidity() const
4   0x11fd530c9 WTF::HashTableConstIterator<WebCore::GraphicsContext3D::Client*, WebCore::GraphicsContext3D::Client*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::GraphicsContext3D::Client*>, WTF::HashTraits<WebCore::GraphicsContext3D::Client*>, WTF::HashTraits<WebCore::GraphicsContext3D::Client*> >::operator++()
5   0x11fd3b503 WTF::HashTableConstIteratorAdapter<WTF::HashTable<WebCore::GraphicsContext3D::Client*, WebCore::GraphicsContext3D::Client*, WTF::IdentityExtractor, WTF::PtrHash<WebCore::GraphicsContext3D::Client*>, WTF::HashTraits<WebCore::GraphicsContext3D::Client*>, WTF::HashTraits<WebCore::GraphicsContext3D::Client*> >, WebCore::GraphicsContext3D::Client*>::operator++()
6   0x11fd3b6e9 WebCore::GraphicsContext3D::recycleContext()
7   0x11fb9a310 WebCore::GraphicsContext3DManager::recycleContextIfNecessary()
8   0x11d0202be WebCore::GraphicsContext3D::create(WebCore::GraphicsContext3DAttributes, WebCore::HostWindow*, WebCore::GraphicsContext3D::RenderStyle)
9   0x120b3a081 WebCore::WebGLRenderingContextBase::create(WebCore::CanvasBase&, WebCore::GraphicsContext3DAttributes&, WTF::String const&)
10  0x11f11baef WebCore::HTMLCanvasElement::createContextWebGL(WTF::String const&, WebCore::GraphicsContext3DAttributes&&)
11  0x11f11a6d4 WebCore::HTMLCanvasElement::getContext(JSC::ExecState&, WTF::String const&, WTF::Vector<JSC::Strong<JSC::Unknown>, 0ul, WTF::CrashOnOverflow, 16ul>&&)
12  0x11d444b7d WebCore::jsHTMLCanvasElementPrototypeFunctionGetContextBody(JSC::ExecState*, WebCore::JSHTMLCanvasElement*, JSC::ThrowScope&)
13  0x11d4350d0 long long WebCore::IDLOperation<WebCore::JSHTMLCanvasElement>::call<&(WebCore::jsHTMLCanvasElementPrototypeFunctionGetContextBody(JSC::ExecState*, WebCore::JSHTMLCanvasElement*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
14  0x11d434dbc WebCore::jsHTMLCanvasElementPrototypeFunctionGetContext(JSC::ExecState*)
15  0x26c17c40116b
16  0x11561af69 llint_entry

https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r243373%20(2055)/results.html

[Ryan Haddad]

Reverted r243356 for reason:

Causes assertion failures with WebGL layout tests on macOS and iOS.

Committed r243383: <https://trac.webkit.org/changeset/243383>

[Devin Rousso]

Created attachment 365747 [details]
Patch

Needed to copy the `GraphicsContext3D`'s list of clients before iterating it :(

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 365658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365658&action=review
> 
> > Source/WebCore/inspector/InspectorCanvas.cpp:386
> > +    m_bufferUsed -= m_actionNeedingSnapshot->memoryCost();
> > +
> > +    ErrorString ignored;
> > +    m_actionNeedingSnapshot->addItem(indexForData(getCanvasContentAsDataURL(ignored)));
> > +
> > +    m_bufferUsed += m_actionNeedingSnapshot->memoryCost();
> 
> This adjusting with memoryCost() is new. `m_bufferUsed` is an unsigned type
> `size_t` should we be worried about potential underflowing/overflowing or is
> the bufferUsed guaranteed to already contain at least action needing
> snapshot? It seems this arithmetic is happening to allow this to happen at
> no cost?

(In reply to Devin Rousso from comment #6)
> Comment on attachment 365658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365658&action=review
> 
> >> Source/WebCore/inspector/InspectorCanvas.cpp:386
> >> +    m_bufferUsed += m_actionNeedingSnapshot->memoryCost();
> > 
> > This adjusting with memoryCost() is new. `m_bufferUsed` is an unsigned type `size_t` should we be worried about potential underflowing/overflowing or is the bufferUsed guaranteed to already contain at least action needing snapshot? It seems this arithmetic is happening to allow this to happen at no cost?
> 
> I don't think we realistically need to worry about overflow.

I was more worried about underflow in the first statement:

   m_bufferUsed -= m_actionNeedingSnapshot->memoryCost();

Which would then overflow on the way back (+=).

[Devin Rousso]

Comment on attachment 365658 [details]
Patch

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

>>>>> Source/WebCore/inspector/InspectorCanvas.cpp:386
>>>>> +    m_bufferUsed += m_actionNeedingSnapshot->memoryCost();
>>>> 
>>>> This adjusting with memoryCost() is new. `m_bufferUsed` is an unsigned type `size_t` should we be worried about potential underflowing/overflowing or is the bufferUsed guaranteed to already contain at least action needing snapshot? It seems this arithmetic is happening to allow this to happen at no cost?
>>> 
>>> I don't think we realistically need to worry about overflow.  In order for that to happen, given that `m_actionNeedingSnapshot` is a serialized JSON value, we'd need to have a JSON file that's over 4GB in size.  At that point, Web Inspector itself would run out of memory.  Yes, we could impose some sort of limit, but there's realistically no single action that can be performed by a canvas (right now) that would result in such a massive payload (yes, someone could try to `drawImage` with some <img> that's massive, but at that point I'd imagine that there would be bigger problems).  Regardless, we default the buffer limit to 100MB (unless otherwise set by `console.record`), so we should be safe.
>>> 
>>> TL;DR: no, we shouldn't have to worry about this because that would be such a massive amount of data that it's effectively outside the realm of possibility.
>> 
>> (In reply to Devin Rousso from comment #6)
> 
> I was more worried about underflow in the first statement:
> 
>    m_bufferUsed -= m_actionNeedingSnapshot->memoryCost();
> 
> Which would then overflow on the way back (+=).

Oh!  That for sure won't happen, as `m_actionNeedingSnapshot` is set right after we've added the `memoryCost` to buffer used.

```
    appendActionSnapshotIfNeeded();

    auto action = buildAction(name, WTFMove(parameters));
    m_bufferUsed += action->memoryCost();
    m_currentActions->addItem(action.ptr());

    if (is<ImageBitmapRenderingContext>(m_context) && shouldSnapshotBitmapRendererAction(name))
        m_actionNeedingSnapshot = WTFMove(action);
#if ENABLE(WEBGL)
    else if (is<WebGLRenderingContext>(m_context) && shouldSnapshotWebGLAction(name))
        m_actionNeedingSnapshot = WTFMove(action);
#endif
```

`m_actionNeedingSnapshot` isn't modified after it's set until `appendActionSnapshotIfNeeded`.  As such, we're always guaranteed to have added the previous `memoryCost`, so us subtracting it here won't underflow.

[WebKit Commit Bot]

Comment on attachment 365747 [details]
Patch

Clearing flags on attachment: 365747

Committed r243400: <https://trac.webkit.org/changeset/243400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

> Oh!  That for sure won't happen, as `m_actionNeedingSnapshot` is set right
> after we've added the `memoryCost` to buffer used.

Thats what I thought, I just wanted to make sure. Thanks for following up!