Summary: | [GPU Process] Canvas drawing never releases any fonts | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||
Component: | Canvas | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | dino, jonlee, mmaxfield, sabouhallawa, simon.fraser, thorton, tsavell, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=227229 | ||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2021-06-24 14:00:44 PDT
This came from https://bugs.webkit.org/show_bug.cgi?id=227229 Created attachment 432314 [details]
Patch
Comment on attachment 432314 [details] Patch Getting the same crashes that appeared in https://bugs.webkit.org/show_bug.cgi?id=227229 Can reproduce the crash with: run-webkit-tests --debug --no-retry --child-processes=1 --verbose fast/canvas/2d.fillText.gradient.html fast/canvas/2d.getPath.modification.html I can reproduce a crash (which may be a different crash) with this: DYLD_FRAMEWORK_PATH=/Users/mmaxfield/Build/Products/Debug ~/Build/Products/Debug/WebKitTestRunner "file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/canvas/2d.fillText.gradient.html" "file:///Users/mmaxfield/src/WebKit/OpenSource/LayoutTests/fast/canvas/2d.getPath.modification.html" My theory is that the web process is sending the releaseRemoteResource() message, and then quitting, then a new web process starts up (for the second test, because it has <!-- webkit-test-runner [ InspectorAdditionsEnabled=true ] -->) it creates a new context, and somehow the old message is being delivered to the new context. The new context says "well I don't have any fonts cached with ID 19" and hits the ASSERT_NOT_REACHED(). If this theory is correct, this is real bad. I think my theory is confirmed. From the point of view of the GPU Process, we see a RemoteRenderingBackend being created with destinationID 16, then it's destroyed, then we see another RemoteRenderingBackend being created with destinationID 16. Created attachment 433007 [details]
Patch
Comment on attachment 433007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433007&action=review > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:152 > + if (unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount && unusedFontCount <= maximumUnusedFontCountToSkipRemoval) { > + prepareForNextRenderingUpdate(); It would be clearer to move all the font releasing code into a new function, so that this early return is less confusing. It would be too easy to add code below that is not font related. Created attachment 433083 [details]
Patch
Comment on attachment 433083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433083&action=review Do we need to change RemoteResourceCacheProxy::cacheFont()? See your comment about it in https://bugs.webkit.org/show_bug.cgi?id=227229#c3 and consider when m_currentRenderingUpdateVersion is zero? > Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:152 > if (unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount && unusedFontCount <= maximumUnusedFontCountToSkipRemoval) > return; Do we need to change this if-statement or change maximumUnusedFontCountToSkipRemoval from being zero? (In reply to Said Abou-Hallawa from comment #12) > Comment on attachment 433083 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433083&action=review > > Do we need to change RemoteResourceCacheProxy::cacheFont()? See your comment > about it in https://bugs.webkit.org/show_bug.cgi?id=227229#c3 and consider > when m_currentRenderingUpdateVersion is zero? Nope, because this patch doesn't reset m_currentRenderingUpdateVersion to zero. m_currentRenderingUpdateVersion is a uint64_t so we aren't in danger of it overflowing. Comment on attachment 433083 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433083&action=review >> Source/WebKit/WebProcess/GPU/graphics/RemoteResourceCacheProxy.cpp:152 >> return; > > Do we need to change this if-statement or change maximumUnusedFontCountToSkipRemoval from being zero? Good idea. I'll upload a new patch. Created attachment 433236 [details]
Patch
(In reply to Myles C. Maxfield from comment #13) > (In reply to Said Abou-Hallawa from comment #12) > > Comment on attachment 433083 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=433083&action=review > > > > Do we need to change RemoteResourceCacheProxy::cacheFont()? See your comment > > about it in https://bugs.webkit.org/show_bug.cgi?id=227229#c3 and consider > > when m_currentRenderingUpdateVersion is zero? > > Nope, because this patch doesn't reset m_currentRenderingUpdateVersion to > zero. m_currentRenderingUpdateVersion is a uint64_t so we aren't in danger > of it overflowing. I just realized we initialize m_currentRenderingUpdateVersion with { 1 }. My question was based on the assumption that it is initialized with { 0 }. Committed r279806 (239570@main): <https://commits.webkit.org/239570@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433236 [details]. It looks like the changes in https://trac.webkit.org/changeset/279806/webkit Broke two tests: inspector/canvas/recording-webgl-snapshots.html inspector/canvas/requestClientNodes-css.html History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=inspector%2Fcanvas%2Frecording-webgl-snapshots.html&test=inspector%2Fcanvas%2FrequestClientNodes-css.html tracking in https://bugs.webkit.org/show_bug.cgi?id=227929 |