Bug 196082

Summary: Web Inspector: Safari Canvas Inspector seems to show the canvas being rendered twice per frame.
Product: WebKit Reporter: david.bismut
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, hi, inspector-bugzilla-changes, joepeck, jonlee, kondapallykalyan, noam, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: All   
OS: All   
Attachments:
Description Flags
Capture of the canvas inspector
none
Patch
none
Patch none

Description david.bismut 2019-03-21 09:44:26 PDT
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
Comment 1 Radar WebKit Bug Importer 2019-03-21 10:46:00 PDT
<rdar://problem/49113496>
Comment 2 Devin Rousso 2019-03-21 17:55:56 PDT
Created attachment 365658 [details]
Patch
Comment 3 WebKit Commit Bot 2019-03-21 19:47:41 PDT
Comment on attachment 365658 [details]
Patch

Clearing flags on attachment: 365658

Committed r243356: <https://trac.webkit.org/changeset/243356>
Comment 4 WebKit Commit Bot 2019-03-21 19:47:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Joseph Pecoraro 2019-03-21 20:23:57 PDT
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?
Comment 6 Devin Rousso 2019-03-21 23:20:06 PDT
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.
Comment 7 Ryan Haddad 2019-03-22 08:54:57 PDT
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
Comment 8 Ryan Haddad 2019-03-22 09:55:41 PDT
Reverted r243356 for reason:

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

Committed r243383: <https://trac.webkit.org/changeset/243383>
Comment 9 Devin Rousso 2019-03-22 11:11:32 PDT
Created attachment 365747 [details]
Patch

Needed to copy the `GraphicsContext3D`'s list of clients before iterating it :(
Comment 10 Joseph Pecoraro 2019-03-22 11:42:01 PDT
(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 (+=).
Comment 11 Devin Rousso 2019-03-22 11:50:03 PDT
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.
Comment 12 WebKit Commit Bot 2019-03-22 15:03:28 PDT
Comment on attachment 365747 [details]
Patch

Clearing flags on attachment: 365747

Committed r243400: <https://trac.webkit.org/changeset/243400>
Comment 13 WebKit Commit Bot 2019-03-22 15:03:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Joseph Pecoraro 2019-03-22 15:12:13 PDT
> 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!