115131 2013-04-24 15:09:56 -0700 REGRESSION (r146544): Once a custom font is cached to disk, it starts failing to render until the page is refreshed. 2013-04-24 17:12:27 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Mac Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 beidson beidson ap commit-queue japhet mitz sam oldest_to_newest 880818 0 beidson 2013-04-24 15:09:56 -0700 REGRESSION (r146544): Once a custom font is cached to disk, it starts failing to render until the page is refreshed. One example of a WebFont user in the wild is http://www.okemo.com/activities/snowsports/ticketing/deals/onlinedeals.asp We can't do the CachedResource::tryReplaceEncodedData trick for CachedFonts on CG platforms because the CGFont object is directly reliant on the original encoded data from the original SharedBuffer. This patch will introduce the concept of a CachedResource saying it doesn't support being swapped out. In radar as <rdar://problem/13622998> 880822 1 199521 beidson 2013-04-24 15:15:31 -0700 Created attachment 199521 Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object 880825 2 199521 ap 2013-04-24 15:25:04 -0700 Comment on attachment 199521 Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > Source/WebCore/loader/cache/CachedFont.h:67 > + virtual bool shouldTryReplaceEncodedData() const; I think that it's a "may", not "should". > Source/WebCore/loader/cache/CachedResource.h:299 > virtual PurgePriority purgePriority() const { return PurgeDefault; } > + virtual bool shouldTryReplaceEncodedData() const { return true; } OVERRIDE 880826 3 beidson 2013-04-24 15:25:58 -0700 (In reply to comment #2) > (From update of attachment 199521 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > > Source/WebCore/loader/cache/CachedFont.h:67 > > + virtual bool shouldTryReplaceEncodedData() const; > > I think that it's a "may", not "should". Fair enough. > > > Source/WebCore/loader/cache/CachedResource.h:299 > > virtual PurgePriority purgePriority() const { return PurgeDefault; } > > + virtual bool shouldTryReplaceEncodedData() const { return true; } > > OVERRIDE Duh. Thanks! 880830 4 beidson 2013-04-24 15:34:49 -0700 http://trac.webkit.org/changeset/149070 880840 5 199521 mitz 2013-04-24 15:43:36 -0700 Comment on attachment 199521 Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review >>> Source/WebCore/loader/cache/CachedResource.h:299 >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } >> >> OVERRIDE > > Duh. > > Thanks! I’m surprised that the default is true! 880854 6 beidson 2013-04-24 16:00:41 -0700 (In reply to comment #5) > (From update of attachment 199521 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > >>> Source/WebCore/loader/cache/CachedResource.h:299 > >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } > >> > >> OVERRIDE > > > > Duh. > > > > Thanks! > > I’m surprised that the default is true! Why? Only certain CachedFonts are known to misbehave with the replacement. All other CachedResources still get it. 880857 7 mitz 2013-04-24 16:06:01 -0700 (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 199521 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > > > >>> Source/WebCore/loader/cache/CachedResource.h:299 > > >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } > > >> > > >> OVERRIDE > > > > > > Duh. > > > > > > Thanks! > > > > I’m surprised that the default is true! > > Why? Only certain CachedFonts are known to misbehave with the replacement. All other CachedResources still get it. Because if you’re writing a new CachedResource derived class, you should inherit safe behavior by default, and make a conscious choice to opt into unsafe behavior if you know that you can handle it. 880859 8 beidson 2013-04-24 16:08:42 -0700 (In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (From update of attachment 199521 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=199521&action=review > > > > > > >>> Source/WebCore/loader/cache/CachedResource.h:299 > > > >>> + virtual bool shouldTryReplaceEncodedData() const { return true; } > > > >> > > > > > > I’m surprised that the default is true! > > > > Why? Only certain CachedFonts are known to misbehave with the replacement. All other CachedResources still get it. > > Because if you’re writing a new CachedResource derived class, you should inherit safe behavior by default, and make a conscious choice to opt into unsafe behavior if you know that you can handle it. Oh, you and your good points. :P 880861 9 beidson 2013-04-24 16:10:53 -0700 But on that note, I think it should probably be pure virtual so you're forced to consider it. 880872 10 ap 2013-04-24 16:31:22 -0700 I would note that "safe" behavior is the one that doesn't kill performance. From this point of view, the current default is actually safe. 880880 11 mitz 2013-04-24 16:56:33 -0700 (In reply to comment #10) > I would note that "safe" behavior is the one that doesn't kill performance. From this point of view, the current default is actually safe. When I used the word “safe”, I didn’t mean “doesn’t kill performance”. Maybe there’s another word for “doesn’t kill performance”, or maybe there should be, but the word “safe” already means something different, as in the sentence “when shouldTryReplaceEncodedData() returns false, it is not safe to hang on to the pointer to the encoded data beyond the end of the current iteration of the run loop”. 880882 12 mitz 2013-04-24 16:58:56 -0700 (In reply to comment #11) > (In reply to comment #10) > > I would note that "safe" behavior is the one that doesn't kill performance. From this point of view, the current default is actually safe. > > When I used the word “safe”, I didn’t mean “doesn’t kill performance”. Maybe there’s another word for “doesn’t kill performance”, or maybe there should be, but the word “safe” already means something different, as in the sentence “when shouldTryReplaceEncodedData() returns false, it is not safe to hang on to the pointer to the encoded data beyond the end of the current iteration of the run loop”. s/false/true/ :-\ 880888 13 beidson 2013-04-24 17:12:27 -0700 This whole mechanism is a memory optimization for process-per-tab mode. Not doing it does not "kill performance", and *all* resources will still get the file backed memory on the next load after it was cached. 199521 2013-04-24 15:15:31 -0700 2013-04-24 15:43:36 -0700 Patch v1 - Don't replace encoded data for a CachedFont that has ever created a FontCustomPlatformData object patch text/plain 4428 beidson ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxN2MxZmM2Li45NWNmMjkyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjQg QEAKKzIwMTMtMDQtMjQgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgorCisgICAg ICAgIE9uY2UgYSBjdXN0b20gZm9udCBpcyBjYWNoZWQgdG8gZGlzaywgaXQgc3RhcnRzIGZhaWxp bmcgdG8gcmVuZGVyIHVudGlsIHRoZSBwYWdlIGlzIHJlZnJlc2hlZC4KKyAgICAgICAgPHJkYXI6 Ly9wcm9ibGVtLzEzNjIyOTk4PiBhbmQgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTExNTEzMQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIE5vIG5ldyB0ZXN0cyAoTm90IGEgdGVzdGVkIGNvbmZpZywgbm9yIGFyZSBkaXNrIGNh Y2hlIGlzc3VlcyBjdXJyZW50bHkgdGVzdGFibGUpLgorCisgICAgICAgICogbG9hZGVyL2NhY2hl L0NhY2hlZFJlc291cmNlLmg6CisgICAgICAgIChXZWJDb3JlOjpDYWNoZWRSZXNvdXJjZTo6c2hv dWxkVHJ5UmVwbGFjZUVuY29kZWREYXRhKTogQWxsb3cgc3ViY2xhc3NlcyB0byByZWZ1c2UgZW5j b2RlZCBkYXRhIHJlcGxhY2VtZW50LgorCisgICAgICAgICogbG9hZGVyL2NhY2hlL0NhY2hlZFJl c291cmNlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNhY2hlZFJlc291cmNlOjp0cnlSZXBsYWNl RW5jb2RlZERhdGEpOiBPbmx5IHRyeSBpZiBzaG91bGRUcnlSZXBsYWNlRW5jb2RlZERhdGEoKSBp cyB0cnVlLgorCisgICAgICAgICogbG9hZGVyL2NhY2hlL0NhY2hlZEZvbnQuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6Q2FjaGVkRm9udDo6Q2FjaGVkRm9udCk6CisgICAgICAgIChXZWJDb3JlOjpD YWNoZWRGb250OjplbnN1cmVDdXN0b21Gb250RGF0YSk6CisgICAgICAgIChXZWJDb3JlOjpDYWNo ZWRGb250OjpzaG91bGRUcnlSZXBsYWNlRW5jb2RlZERhdGEpOiBSZXR1cm4gZmFsc2UgaWYgdGhl IGN1c3RvbSBmb250IGRhdGEgaGFzIGV2ZXIgYmVlbiBjcmVhdGVkLgorICAgICAgICAqIGxvYWRl ci9jYWNoZS9DYWNoZWRGb250Lmg6CisKIDIwMTMtMDQtMjMgIFNpbW9uIEZyYXNlciAgPHNpbW9u LmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgRG9uJ3QgY3JlYXRlIGNvbXBvc2l0aW5nIGxh eWVycyBmb3Igc3RpY2t5IHBvc2l0aW9uIHVubGVzcyB1c2luZyB0aGUgU2Nyb2xsaW5nQ29vcmRp bmF0b3IKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9DYWNoZWRGb250 LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9DYWNoZWRGb250LmNwcAppbmRleCA4 NDNjZWNmLi4yNmQ1NGJjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUv Q2FjaGVkRm9udC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL2NhY2hlL0NhY2hlZEZv bnQuY3BwCkBAIC01Miw2ICs1Miw3IEBAIENhY2hlZEZvbnQ6OkNhY2hlZEZvbnQoY29uc3QgUmVz b3VyY2VSZXF1ZXN0JiByZXNvdXJjZVJlcXVlc3QpCiAgICAgOiBDYWNoZWRSZXNvdXJjZShyZXNv dXJjZVJlcXVlc3QsIEZvbnRSZXNvdXJjZSkKICAgICAsIG1fZm9udERhdGEoMCkKICAgICAsIG1f bG9hZEluaXRpYXRlZChmYWxzZSkKKyAgICAsIG1faGFzQ3JlYXRlZEZvbnREYXRhKGZhbHNlKQog ewogfQogCkBAIC05Nyw3ICs5OCw5IEBAIGJvb2wgQ2FjaGVkRm9udDo6ZW5zdXJlQ3VzdG9tRm9u dERhdGEoKQogewogICAgIGlmICghbV9mb250RGF0YSAmJiAhZXJyb3JPY2N1cnJlZCgpICYmICFp c0xvYWRpbmcoKSAmJiBtX2RhdGEpIHsKICAgICAgICAgbV9mb250RGF0YSA9IGNyZWF0ZUZvbnRD dXN0b21QbGF0Zm9ybURhdGEobV9kYXRhLmdldCgpLT5zaGFyZWRCdWZmZXIoKSk7Ci0gICAgICAg IGlmICghbV9mb250RGF0YSkKKyAgICAgICAgaWYgKG1fZm9udERhdGEpCisgICAgICAgICAgICBt X2hhc0NyZWF0ZWRGb250RGF0YSA9IHRydWU7CisgICAgICAgIGVsc2UKICAgICAgICAgICAgIHNl dFN0YXR1cyhEZWNvZGVFcnJvcik7CiAgICAgfQogICAgIHJldHVybiBtX2ZvbnREYXRhOwpAQCAt MTgwLDQgKzE4MywxMyBAQCB2b2lkIENhY2hlZEZvbnQ6OmNoZWNrTm90aWZ5KCkKICAgICAgICAg IGMtPmZvbnRMb2FkZWQodGhpcyk7CiB9CiAKK2Jvb2wgQ2FjaGVkRm9udDo6c2hvdWxkVHJ5UmVw bGFjZUVuY29kZWREYXRhKCkgY29uc3QKK3sKKyAgICAvLyBJZiB0aGUgRm9udEN1c3RvbVBsYXRm b3JtRGF0YSBoYXMgZXZlciBiZWVuIGNvbnN0cnVjdGVkIHRoZW4gaXQgc3RpbGwgbWlnaHQgYmUg aW4gdXNlIHNvbWV3aGVyZS4KKyAgICAvLyBUaGF0IHBsYXRmb3JtIGZvbnQgb2JqZWN0IG1pZ2h0 IGRpcmVjdGx5IHJlZmVyZW5jZSB0aGUgZW5jb2RlZCBkYXRhIGJ1ZmZlciBiZWhpbmQgdGhpcyBD YWNoZWRGb250LAorICAgIC8vIHNvIHJlcGxhY2luZyBpdCBpcyB1bnNhZmUuCisKKyAgICByZXR1 cm4gIW1faGFzQ3JlYXRlZEZvbnREYXRhOworfQorCiB9CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkRm9udC5oIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL2Nh Y2hlL0NhY2hlZEZvbnQuaAppbmRleCA3YTUzYWViLi4yNGM0YmJiIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkRm9udC5oCisrKyBiL1NvdXJjZS9XZWJDb3Jl L2xvYWRlci9jYWNoZS9DYWNoZWRGb250LmgKQEAgLTY0LDggKzY0LDExIEBAIHB1YmxpYzoKIAog cHJpdmF0ZToKICAgICB2aXJ0dWFsIHZvaWQgY2hlY2tOb3RpZnkoKTsKKyAgICB2aXJ0dWFsIGJv b2wgc2hvdWxkVHJ5UmVwbGFjZUVuY29kZWREYXRhKCkgY29uc3Q7CisKICAgICBGb250Q3VzdG9t UGxhdGZvcm1EYXRhKiBtX2ZvbnREYXRhOwogICAgIGJvb2wgbV9sb2FkSW5pdGlhdGVkOworICAg IGJvb2wgbV9oYXNDcmVhdGVkRm9udERhdGE7CiAKICNpZiBFTkFCTEUoU1ZHX0ZPTlRTKQogICAg IFJlZlB0cjxTVkdEb2N1bWVudD4gbV9leHRlcm5hbFNWR0RvY3VtZW50OwpkaWZmIC0tZ2l0IGEv U291cmNlL1dlYkNvcmUvbG9hZGVyL2NhY2hlL0NhY2hlZFJlc291cmNlLmNwcCBiL1NvdXJjZS9X ZWJDb3JlL2xvYWRlci9jYWNoZS9DYWNoZWRSZXNvdXJjZS5jcHAKaW5kZXggNWI5MjllYy4uZjVm ZjZhZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvbG9hZGVyL2NhY2hlL0NhY2hlZFJlc291 cmNlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkUmVzb3VyY2Uu Y3BwCkBAIC05MDYsNiArOTA2LDkgQEAgdm9pZCBDYWNoZWRSZXNvdXJjZTo6dHJ5UmVwbGFjZUVu Y29kZWREYXRhKFBhc3NSZWZQdHI8U2hhcmVkQnVmZmVyPiBuZXdCdWZmZXIpCiAgICAgaWYgKCFt X2RhdGEpCiAgICAgICAgIHJldHVybjsKICAgICAKKyAgICBpZiAoIXNob3VsZFRyeVJlcGxhY2VF bmNvZGVkRGF0YSgpKQorICAgICAgICByZXR1cm47CisgICAgCiAgICAgLy8gQmVjYXVzZSB0aGUg ZGlzayBjYWNoZSBpcyBhc3luY2hyb25vdXMgYW5kIHJhY2V5IHdpdGggcmVnYXJkcyB0byB0aGUg ZGF0YSB3ZSBtaWdodCBiZSBhc2tlZCB0byByZXBsYWNlLAogICAgIC8vIHdlIG5lZWQgdG8gdmVy aWZ5IHRoYXQgdGhlIG5ldyBidWZmZXIgaGFzIHRoZSBzYW1lIGNvbnRlbnRzIGFzIG91ciBvbGQg YnVmZmVyLgogICAgIGlmIChtX2RhdGEtPnNpemUoKSAhPSBuZXdCdWZmZXItPnNpemUoKSB8fCBt ZW1jbXAobV9kYXRhLT5kYXRhKCksIG5ld0J1ZmZlci0+ZGF0YSgpLCBtX2RhdGEtPnNpemUoKSkp CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvY2FjaGUvQ2FjaGVkUmVzb3VyY2Uu aCBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9DYWNoZWRSZXNvdXJjZS5oCmluZGV4IDcz MDQzNjUuLjNmYzIzOTcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9D YWNoZWRSZXNvdXJjZS5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9jYWNoZS9DYWNoZWRS ZXNvdXJjZS5oCkBAIC0yOTYsNiArMjk2LDcgQEAgcHJpdmF0ZToKICAgICB2b2lkIGRlY29kZWRE YXRhRGVsZXRpb25UaW1lckZpcmVkKFRpbWVyPENhY2hlZFJlc291cmNlPiopOwogCiAgICAgdmly dHVhbCBQdXJnZVByaW9yaXR5IHB1cmdlUHJpb3JpdHkoKSBjb25zdCB7IHJldHVybiBQdXJnZURl ZmF1bHQ7IH0KKyAgICB2aXJ0dWFsIGJvb2wgc2hvdWxkVHJ5UmVwbGFjZUVuY29kZWREYXRhKCkg Y29uc3QgeyByZXR1cm4gdHJ1ZTsgfQogCiAgICAgZG91YmxlIGN1cnJlbnRBZ2UoKSBjb25zdDsK ICAgICBkb3VibGUgZnJlc2huZXNzTGlmZXRpbWUoKSBjb25zdDsK