In this function 'unusedFontCount' is always equal to zero because m_fontIdentifierToLastRenderingUpdateVersionMap.size() is always equal to m_numberOfFontsUsedInCurrentRenderingUpdate. In other words all the cached fonts are always considered be used by the current RenderingUpdate. So this function makes an early return through the if-statement: if (unusedFontCount < minimumFractionOfUnusedFontCountToTriggerRemoval * totalFontCount && unusedFontCount <= maximumUnusedFontCountToSkipRemoval) return; except in one case which is totalFontCount == 0. In this case, the body of this function will be executed but it will do nothing because m_fontIdentifierToLastRenderingUpdateVersionMap is empty. This means all the fonts are cached always in GPUP. They are removed only under memory pressure through WebPage::releaseMemory() which calls RemoteResourceCacheProxy::releaseMemory().
<rdar://problem/79741186>
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