Summary: | Web Inspector: Safari Canvas Inspector seems to show the canvas being rendered twice per frame. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | david.bismut | ||||||||
Component: | Web Inspector | Assignee: | 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
david.bismut
2019-03-21 09:44:26 PDT
Created attachment 365658 [details]
Patch
Comment on attachment 365658 [details] Patch Clearing flags on attachment: 365658 Committed r243356: <https://trac.webkit.org/changeset/243356> All reviewed patches have been landed. Closing bug. 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 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. 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 Reverted r243356 for reason: Causes assertion failures with WebGL layout tests on macOS and iOS. Committed r243383: <https://trac.webkit.org/changeset/243383> Created attachment 365747 [details]
Patch
Needed to copy the `GraphicsContext3D`'s list of clients before iterating it :(
(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 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 on attachment 365747 [details] Patch Clearing flags on attachment: 365747 Committed r243400: <https://trac.webkit.org/changeset/243400> All reviewed patches have been landed. Closing bug. > 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!
|