189722 2018-09-18 16:13:58 -0700 Fix crash under FontCache::purgeInactiveFontData() when a memory warning fires 2018-10-04 09:40:21 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=189861 InRadar P2 Normal --- 1 simon.fraser simon.fraser cdumez commit-queue darin mmaxfield simon.fraser webkit-bug-importer oldest_to_newest 1460795 0 simon.fraser 2018-09-18 16:13:58 -0700 Fix crash under FontCache::purgeInactiveFontData() when a memory warning fires 1460800 1 350073 simon.fraser 2018-09-18 16:18:08 -0700 Created attachment 350073 Patch 1460801 2 simon.fraser 2018-09-18 16:18:10 -0700 <rdar://problem/44182860> 1460859 3 350073 mmaxfield 2018-09-18 23:45:48 -0700 Comment on attachment 350073 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. 1461276 4 350073 mmaxfield 2018-09-20 01:34:34 -0700 Comment on attachment 350073 Patch I can't reproduce this in the simulator, so we can commit this until I get back to Cupertino. 1461278 5 350073 commit-queue 2018-09-20 02:00:22 -0700 Comment on attachment 350073 Patch Clearing flags on attachment: 350073 Committed r236254: <https://trac.webkit.org/changeset/236254> 1461279 6 commit-queue 2018-09-20 02:00:23 -0700 All reviewed patches have been landed. Closing bug. 1461487 7 darin 2018-09-20 12:46:29 -0700 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. 1464459 8 350073 cdumez 2018-09-28 12:11:29 -0700 Comment on attachment 350073 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? 1464460 9 cdumez 2018-09-28 12:13:20 -0700 (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. 1464574 10 350073 darin 2018-09-28 15:53:03 -0700 Comment on attachment 350073 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. 1466098 11 cdumez 2018-10-03 10:09:17 -0700 (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. 1466184 12 mmaxfield 2018-10-03 14:35:40 -0700 (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. 1466193 13 cdumez 2018-10-03 15:02:35 -0700 (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. 1466218 14 mmaxfield 2018-10-03 16:11:45 -0700 (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. 1466220 15 cdumez 2018-10-03 16:15:03 -0700 (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. 1466379 16 darin 2018-10-04 09:40:21 -0700 I believe Chris’s analysis is correct. There is no need to "revert" this patch. 350073 2018-09-18 16:18:08 -0700 2018-09-20 02:00:22 -0700 Patch bug-189722-20180918161806.patch text/plain 2384 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMjM2MDk0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzE1Njc1YjBmZjMyNjE3 YmNiNDMyNTg4NTg1NGJjMjRjNjY3OTg3Zi4uNWJmNjUyMWNlMDg0ZDM4YTFkMDVhNTlmNzZlM2Q0 MjU2MDdhNDdkMyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2IEBACisyMDE4LTA5LTE4ICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgorCisgICAgICAgIEZpeCBjcmFzaCB1 bmRlciBGb250Q2FjaGU6OnB1cmdlSW5hY3RpdmVGb250RGF0YSgpIHdoZW4gYSBtZW1vcnkgd2Fy bmluZyBmaXJlcworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTg5NzIyCisgICAgICAgIHJkYXI6Ly9wcm9ibGVtLzQ0MTgyODYwCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisgICAgICAgIAorICAgICAgICBIYXNoaW5nIG9mIEZv bnRQbGF0Zm9ybURhdGEgZm9yIGNhY2hlZEZvbnRzKCkgaXMgc29tZXdoYXQgYnJva2VuIGJlY2F1 c2UgQ0ZFcXVhbCgpIG9uIENURm9udAorICAgICAgICBjYW4gcmV0dXJuIGZhbHNlIHdoZW4gdGhl IGZvbnRzIGFyZSBhY3R1YWxseSB0aGUgc2FtZSwgYW5kIGhhdmUgdGhlIHNhbWUgQ0ZIYXNoKCku IFRoaXMgCisgICAgICAgIGNhbiByZXN1bHQgaW4gbXVsdGlwbGUgZW50cmllcyBpbiBjYWNoZWRG b250cygpIHdpdGggdGhlIHNhbWUgRm9udC4KKyAgICAgICAgCisgICAgICAgIFRoZW4gaW4gRm9u dENhY2hlOjpwdXJnZUluYWN0aXZlRm9udERhdGEoKSwgdGhlIGxvb3AgdGhhdCBhcHBlbmRzIGZv bnRzIHRvIGZvbnRzVG9EZWxldGUKKyAgICAgICAgZ2V0cyB0aGUgdmFsdWUgYnkgcmVmZXJlbmNl LCBhbmQgV1RGTW92ZXMgaXQgaW50byBmb250c1RvRGVsZXRlLiBUaGlzIG51bGxzIG91dCBhbGwK KyAgICAgICAgdGhlIGVudHJpZXMgc2hhcmluZyB0aGUgc2FtZSB2YWx1ZSwgbGVhdmluZyBudWxs IGVudHJpZXMgaW4gdGhlIGhhc2ggdGFibGUuCisgICAgICAgIFdlIGxhdGVyIGNyYXNoIGF0IGZv bnQtPmhhc09uZVJlZigpIHdoZW4gdXNpbmcgb25lIG9mIHRob3NlIG51bGwgZW50cmllcy4KKyAg ICAgICAgCisgICAgICAgIEZpeCBieSBtYWtpbmcgYSBjb3B5IG9mIHRoZSBSZWZQdHI8Rm9udD4g aW4gdGhlIGxvb3AsIHNvIHRoZSBXVEZNb3ZlIGRvZXNuJ3QgbnVrZQorICAgICAgICB0aGUgaGFz aCB0YWJsZSBlbnRyaWVzLiBUaGUgZW50cmllcyB3aWxsIGdldCByZW1vdmVkIGF0IGNhY2hlZEZv bnRzKCkucmVtb3ZlKCkgbG93ZXIgZG93bi4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNz L0ZvbnRDYWNoZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpGb250Q2FjaGU6OnB1cmdlSW5hY3Rp dmVGb250RGF0YSk6CisKIDIwMTgtMDktMTcgIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5j b20+CiAKICAgICAgICAgQWRkIHN1cHBvcnQgZm9yIEhFVkMgY29kZWMgdHlwZXMgaW4gTWVkaWEg Q2FwYWJpbGl0aWVzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy9Gb250Q2FjaGUuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvRm9udENh Y2hlLmNwcAppbmRleCA1ODc3M2JlNzNjMjZjMzc5MTM5ODA5MGJkNTQwYjNhOTIwYWQ1NmJmLi40 YmJhZGZiMjgzMDBlNGNmMTJlYjJlODJiNmU5ZWE2MmI2YjE4ODVmIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9Gb250Q2FjaGUuY3BwCisrKyBiL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0ZvbnRDYWNoZS5jcHAKQEAgLTM3Niw3ICszNzYsNyBA QCB2b2lkIEZvbnRDYWNoZTo6cHVyZ2VJbmFjdGl2ZUZvbnREYXRhKHVuc2lnbmVkIHB1cmdlQ291 bnQpCiAKICAgICB3aGlsZSAocHVyZ2VDb3VudCkgewogICAgICAgICBWZWN0b3I8UmVmUHRyPEZv bnQ+LCAyMD4gZm9udHNUb0RlbGV0ZTsKLSAgICAgICAgZm9yIChhdXRvJiBmb250IDogY2FjaGVk Rm9udHMoKS52YWx1ZXMoKSkgeworICAgICAgICBmb3IgKGF1dG8gZm9udCA6IGNhY2hlZEZvbnRz KCkudmFsdWVzKCkpIHsKICAgICAgICAgICAgIExPRyhGb250cywgIiB0cnlpbmcgdG8gcHVyZ2Ug Zm9udCAlcyAoaGFzIG9uZSByZWYgJWQpIiwgZm9udC0+cGxhdGZvcm1EYXRhKCkuZGVzY3JpcHRp b24oKS51dGY4KCkuZGF0YSgpLCBmb250LT5oYXNPbmVSZWYoKSk7CiAgICAgICAgICAgICBpZiAo IWZvbnQtPmhhc09uZVJlZigpKQogICAgICAgICAgICAgICAgIGNvbnRpbnVlOwo=