Fix crash under FontCache::purgeInactiveFontData() when a memory warning fires
Created attachment 350073 [details] Patch
<rdar://problem/44182860>
Comment on attachment 350073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350073&action=review > Source/WebCore/platform/graphics/FontCache.cpp:379 > - for (auto& font : cachedFonts().values()) { > + for (auto font : cachedFonts().values()) { Right, I see what's happening here. cachedFonts().remove(font->platformData()); is removing the wrong item. This null deref means that if we follow the RefPtr in the cachedFonts() cache to a Font object, and we look up that Font’s inner FontPlatformData, we don’t end up with the same entry in cachedFonts() than we started with. This should be impossible because we maintain the invariant that this cycle should hold. Copying the RefPtr is the wrong thing to do because it means that the cachedFonts().remove() call won't actually remove the font, which is contrary to the whole point of this function. This is papering over the problem rather than actually fixing it.
Comment on attachment 350073 [details] Patch I can't reproduce this in the simulator, so we can commit this until I get back to Cupertino.
Comment on attachment 350073 [details] Patch Clearing flags on attachment: 350073 Committed r236254: <https://trac.webkit.org/changeset/236254>
All reviewed patches have been landed. Closing bug.
This is an extremely important bug to fix, and I am thrilled that it’s resolved! Since moving out of the hash table value is something we can’t support, I think the best way to change the code to do that is to remove the call to WTFMove (and ideally write a clear comment explaining why we must copy and not move, preventing future people from attempting that optimization). The much more subtle alteration we chose to do, changing from auto& to auto so that the WTFMove won’t move out of the hash map values, does not seem to be as clear. I also don’t fully understand why we need to do it. Can we instead fix the crash by changing the code to not assume the pointer can never be null? I also think we need to either find a way to test this, make the code’s rationale much clearer, or both. And the whole thing about "removing the wrong item" seems like it needs a separate fix -- I assume that it has a symptom even if we don’t crash.
Comment on attachment 350073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350073&action=review > Source/WebCore/platform/graphics/FontCache.cpp:381 > if (!font->hasOneRef()) But then how can this ever be false since you have now copied the RefPtr?
(In reply to Chris Dumez from comment #8) > Comment on attachment 350073 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350073&action=review > > > Source/WebCore/platform/graphics/FontCache.cpp:381 > > if (!font->hasOneRef()) > > But then how can this ever be false since you have now copied the RefPtr? Note that I later re-introduced auto& in http://trac.webkit.org/r236383 (but stopped using WTFMove() later), I wasn't aware of Simon's change until now.
Comment on attachment 350073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350073&action=review >>> Source/WebCore/platform/graphics/FontCache.cpp:381 >>> if (!font->hasOneRef()) >> >> But then how can this ever be false since you have now copied the RefPtr? > > Note that I later re-introduced auto& in http://trac.webkit.org/r236383 (but stopped using WTFMove() later), I wasn't aware of Simon's change until now. I’m pretty sure that, because of the error you are pointing out here, in the time after *this* patch was landed but before r236383 we had a purgeInactiveFontData function that would not delete any fonts! This means we need to find a way to add some tests that verify that our purge function actually does what it is supposed to. So next time we break it we notice that we have done so.
(In reply to Darin Adler from comment #10) > Comment on attachment 350073 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350073&action=review > > >>> Source/WebCore/platform/graphics/FontCache.cpp:381 > >>> if (!font->hasOneRef()) > >> > >> But then how can this ever be false since you have now copied the RefPtr? > > > > Note that I later re-introduced auto& in http://trac.webkit.org/r236383 (but stopped using WTFMove() later), I wasn't aware of Simon's change until now. > > I’m pretty sure that, because of the error you are pointing out here, in the > time after *this* patch was landed but before r236383 we had a > purgeInactiveFontData function that would not delete any fonts! > > This means we need to find a way to add some tests that verify that our > purge function actually does what it is supposed to. So next time we break > it we notice that we have done so. FYI, we did notice because r236254 caused a ~2% PLT regression.
(In reply to Chris Dumez from comment #11) > (In reply to Darin Adler from comment #10) > > Comment on attachment 350073 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=350073&action=review > > > > >>> Source/WebCore/platform/graphics/FontCache.cpp:381 > > >>> if (!font->hasOneRef()) > > >> > > >> But then how can this ever be false since you have now copied the RefPtr? > > > > > > Note that I later re-introduced auto& in http://trac.webkit.org/r236383 (but stopped using WTFMove() later), I wasn't aware of Simon's change until now. > > > > I’m pretty sure that, because of the error you are pointing out here, in the > > time after *this* patch was landed but before r236383 we had a > > purgeInactiveFontData function that would not delete any fonts! > > > > This means we need to find a way to add some tests that verify that our > > purge function actually does what it is supposed to. So next time we break > > it we notice that we have done so. > > FYI, we did notice because r236254 caused a ~2% PLT regression. <rdar://problem/44954219> is for reverting this patch.
(In reply to Myles C. Maxfield from comment #12) > (In reply to Chris Dumez from comment #11) > > (In reply to Darin Adler from comment #10) > > > Comment on attachment 350073 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=350073&action=review > > > > > > >>> Source/WebCore/platform/graphics/FontCache.cpp:381 > > > >>> if (!font->hasOneRef()) > > > >> > > > >> But then how can this ever be false since you have now copied the RefPtr? > > > > > > > > Note that I later re-introduced auto& in http://trac.webkit.org/r236383 (but stopped using WTFMove() later), I wasn't aware of Simon's change until now. > > > > > > I’m pretty sure that, because of the error you are pointing out here, in the > > > time after *this* patch was landed but before r236383 we had a > > > purgeInactiveFontData function that would not delete any fonts! > > > > > > This means we need to find a way to add some tests that verify that our > > > purge function actually does what it is supposed to. So next time we break > > > it we notice that we have done so. > > > > FYI, we did notice because r236254 caused a ~2% PLT regression. > > <rdar://problem/44954219> is for reverting this patch. What sure what you mean, http://trac.webkit.org/r236383 basically already reverted it and recovered the regression.
(In reply to Chris Dumez from comment #13) > (In reply to Myles C. Maxfield from comment #12) > > (In reply to Chris Dumez from comment #11) > > > (In reply to Darin Adler from comment #10) > > > > Comment on attachment 350073 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=350073&action=review > > > > > > > > >>> Source/WebCore/platform/graphics/FontCache.cpp:381 > > > > >>> if (!font->hasOneRef()) > > > > >> > > > > >> But then how can this ever be false since you have now copied the RefPtr? > > > > > > > > > > Note that I later re-introduced auto& in http://trac.webkit.org/r236383 (but stopped using WTFMove() later), I wasn't aware of Simon's change until now. > > > > > > > > I’m pretty sure that, because of the error you are pointing out here, in the > > > > time after *this* patch was landed but before r236383 we had a > > > > purgeInactiveFontData function that would not delete any fonts! > > > > > > > > This means we need to find a way to add some tests that verify that our > > > > purge function actually does what it is supposed to. So next time we break > > > > it we notice that we have done so. > > > > > > FYI, we did notice because r236254 caused a ~2% PLT regression. > > > > <rdar://problem/44954219> is for reverting this patch. > > What sure what you mean, http://trac.webkit.org/r236383 basically already > reverted it and recovered the regression. That patch says copyRef(), so I don't understand how that fixed it.
(In reply to Myles C. Maxfield from comment #14) > (In reply to Chris Dumez from comment #13) > > (In reply to Myles C. Maxfield from comment #12) > > > (In reply to Chris Dumez from comment #11) > > > > (In reply to Darin Adler from comment #10) > > > > > Comment on attachment 350073 [details] > > > > > Patch > > > > > > > > > > View in context: > > > > > https://bugs.webkit.org/attachment.cgi?id=350073&action=review > > > > > > > > > > >>> Source/WebCore/platform/graphics/FontCache.cpp:381 > > > > > >>> if (!font->hasOneRef()) > > > > > >> > > > > > >> But then how can this ever be false since you have now copied the RefPtr? > > > > > > > > > > > > Note that I later re-introduced auto& in http://trac.webkit.org/r236383 (but stopped using WTFMove() later), I wasn't aware of Simon's change until now. > > > > > > > > > > I’m pretty sure that, because of the error you are pointing out here, in the > > > > > time after *this* patch was landed but before r236383 we had a > > > > > purgeInactiveFontData function that would not delete any fonts! > > > > > > > > > > This means we need to find a way to add some tests that verify that our > > > > > purge function actually does what it is supposed to. So next time we break > > > > > it we notice that we have done so. > > > > > > > > FYI, we did notice because r236254 caused a ~2% PLT regression. > > > > > > <rdar://problem/44954219> is for reverting this patch. > > > > What sure what you mean, http://trac.webkit.org/r236383 basically already > > reverted it and recovered the regression. > > That patch says copyRef(), so I don't understand how that fixed it. r236383 fixed the regression introduced by r236254 by reverting it: auto -> auto& so that hasOneRef() check does the right thing again r236383 still addresses the crash that r236254 was trying to fix AFAICT, because it uses copyRef() instead of WTFMove(), thus not modifying the original structure.
I believe Chris’s analysis is correct. There is no need to "revert" this patch.