173473 2017-06-16 09:07:34 -0700 Cleanup IconLoader stuff when a DocumentLoader detaches from its frame 2017-06-19 13:12:46 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 beidson beidson achristensen aestes buildbot cdumez commit-queue dbates japhet oldest_to_newest 1319965 0 beidson 2017-06-16 09:07:34 -0700 Null deref crash in DocumentLoader::finishedLoadingIcon Specifically, deref'ing a null frame. 0 com.apple.WebCore 0x000000010faa91fd WebCore::DocumentLoader::finishedLoadingIcon(WebCore::IconLoader&, WebCore::SharedBuffer*) + 29 1 com.apple.WebCore 0x000000010f8dc3fd WebCore::CachedResource::didAddClient(WebCore::CachedResourceClient&) + 205 2 com.apple.WebCore 0x000000010f8d9ad6 WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient&) + 758 3 com.apple.WebCore 0x000000010f60f870 WebCore::ThreadTimers::sharedTimerFiredInternal() + 176 4 com.apple.WebCore 0x000000010f60f7af WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 I have not been able to reproduce. I've been able to figure out a sequence of events to reproduce this: 0 - Be using an embedding app that uses the icon loading delegate API. 1 - Visit a website that references a site icon. 2 - The embedding app says "yes, load it". 3 - The icon is loaded as a sub resource, and that includes putting it in the memory cache. 4 - Navigate to another page within the same website that will load the same icon. 5 - Embedding app says "yes, load it" 6 - DocumentLoader - still attached to a frame - creates an IconLoader for the icon 7 - The IconLoader goes through the memory cache to get at a CachedResource for the icon, and finds it. 8 - The IconLoader attaches itself to the CachedResource as a CachedResourceClient. CachedResource schedules a timer to deliver callbacks to the IconLoader asynchronously 9 - Before that timer fires, the Frame navigates, so the DocumentLoader detaches itself from the frame. 10 - The timer fires, giving the data to the IconLoader and telling it that it finished. 11 - The IconLoader tells the DocumentLoader it finished, which then tries to call out to the FrameLoader client, but the m_frame is gone. I'm 97% sure this is how this is happening, but cannot reproduce. The timing around steps 8-10 is REALLY tight, and I have not found a way to reproduce - it's just a few tight spins of the runloop. I'm going to spend a little while longer trying to reproduce before I give up and fix this speculatively. 1320762 1 beidson 2017-06-19 11:50:26 -0700 Retitling: Cleanup IconLoader stuff when a DocumentLoader detaches from its frame 1320766 2 313322 beidson 2017-06-19 11:58:35 -0700 Created attachment 313322 Patch 1320798 3 313322 commit-queue 2017-06-19 13:12:44 -0700 Comment on attachment 313322 Patch Clearing flags on attachment: 313322 Committed r218502: <http://trac.webkit.org/changeset/218502> 1320799 4 commit-queue 2017-06-19 13:12:46 -0700 All reviewed patches have been landed. Closing bug. 313322 2017-06-19 11:58:35 -0700 2017-06-19 13:12:44 -0700 Patch bug-173473-20170619115834.patch text/plain 2451 beidson U3VidmVyc2lvbiBSZXZpc2lvbjogMjE4NDkyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWI2YjQzYzk5MDBiYmZj NzkzZGEzY2VkYWE2ZGYwYzYxMjM5MDNiNC4uYjE3MzQzYjRlOWNjYTliMmI3MzYxNWYyZjE5ZTFl ZTQ2NTliZjQyNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDE3LTA2LTE5ICBCcmFk eSBFaWRzb24gIDxiZWlkc29uQGFwcGxlLmNvbT4KKworICAgICAgICBDbGVhbnVwIEljb25Mb2Fk ZXIgc3R1ZmYgd2hlbiBhIERvY3VtZW50TG9hZGVyIGRldGFjaGVzIGZyb20gaXRzIGZyYW1lLgor ICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzE0MTg3NjE+IGFuZCBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTczNDczCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZ IChPT1BTISkuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIChObyBrbm93biBjaGFuZ2UgaW4gYmVo YXZpb3IpCisKKyAgICAgICAgSSBkaXNjb3ZlcmVkIHRoZSBuZWVkIHRvIG1ha2UgdGhlc2UgY2hh bmdlcyBoZXJlIGR1ZSB0byBhIHRyYW5zaWVudCBidWcKKyAgICAgICAgaW50cm9kdWNlZCBpbiBy MjE4MDE1IGJ1dCBhbHJlYWR5IGV4cGxpY2l0bHkgZml4ZWQgaW4gcjIxODQwOS4KKyAgICAgICAg CisgICAgICAgIFRoaXMgY2hhbmdlIGFkZHMgYW4gYXNzZXJ0IHRvIGd1YXJkIGFnYWluc3QgYSBk ZXRhY2hlZCBEb2N1bWVudExvYWRlciBoYXZpbmcgYWN0aXZlIEljb25Mb2FkZXJzLgorCisgICAg ICAgIEl0IGFsc28gY2xlYXJzIG91dCBhbGwgcGVuZGluZyBJY29uTG9hZGVyIGFuZCBpY29uIGxv YWQgZGVjaXNpb25zIHdoZW4gc3RvcExvYWRpbmcoKSBpcyBjYWxsZWQsIAorICAgICAgICBhcyBl dmVuIGF0dGVtcHRpbmcgdG8gc3RhcnQgYW4gaWNvbiBsb2FkIGFmdGVyIGRldGFjaG1lbnQgaXMg YSB3YXN0ZSBvZiBjeWNsZXMuCisgICAgICAgIAorICAgICAgICAqIGxvYWRlci9Eb2N1bWVudExv YWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpEb2N1bWVudExvYWRlcjo6c3RvcExvYWRpbmcp OiBBbHNvIGV4cGxpY2l0bHkgY2xlYXIgYWxsIEljb25Mb2FkZXJzIGFuZCBpY29ucyBwZW5kaW5n CisgICAgICAgICAgbG9hZCBkZWNpc2lvbi4KKyAgICAgICAgKFdlYkNvcmU6OkRvY3VtZW50TG9h ZGVyOjpmaW5pc2hlZExvYWRpbmdJY29uKTogQXNzZXJ0IHRoYXQgdGhpcyBEb2N1bWVudExvYWRl ciBpcyBub3QgZGV0YWNoZWQuCisKIDIwMTctMDYtMTkgIEFkcmlhbiBQZXJleiBkZSBDYXN0cm8g IDxhcGVyZXpAaWdhbGlhLmNvbT4KIAogICAgICAgICBNaXNzaW5nIDxmdW5jdGlvbmFsPiBpbmNs dWRlcyBtYWtlIGJ1aWxkcyBmYWlsIHdpdGggR0NDIDcueApkaWZmIC0tZ2l0IGEvU291cmNlL1dl YkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9E b2N1bWVudExvYWRlci5jcHAKaW5kZXggOWJhYmNlZjMwZDdiZmU3MzRkMmRhMGQ5OWQ3NzRhOTA0 MmMyYTIzZS4uZTgyNWIxNDc1MDdhYjAyZTFjMTFiN2E1NGRjNTI3Y2ViNTBlMTRlMSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcAorKysgYi9Tb3Vy Y2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwCkBAIC0yODIsNiArMjgyLDkgQEAg dm9pZCBEb2N1bWVudExvYWRlcjo6c3RvcExvYWRpbmcoKQogICAgICAgICAgICAgbV9mcmFtZS0+ bG9hZGVyKCkuc3RvcExvYWRpbmcoVW5sb2FkRXZlbnRQb2xpY3lOb25lKTsKICAgICB9CiAKKyAg ICBtX2ljb25zUGVuZGluZ0xvYWREZWNpc2lvbi5jbGVhcigpOworICAgIG1faWNvbkxvYWRlcnMu Y2xlYXIoKTsKKwogICAgIC8vIEFsd2F5cyBjYW5jZWwgbXVsdGlwYXJ0IGxvYWRlcnMKICAgICBj YW5jZWxBbGwobV9tdWx0aXBhcnRTdWJyZXNvdXJjZUxvYWRlcnMpOwogCkBAIC0xNjg5LDYgKzE2 OTIsOSBAQCB2b2lkIERvY3VtZW50TG9hZGVyOjpkaWRHZXRMb2FkRGVjaXNpb25Gb3JJY29uKGJv b2wgZGVjaXNpb24sIHVpbnQ2NF90IGxvYWRJZGVudAogCiB2b2lkIERvY3VtZW50TG9hZGVyOjpm aW5pc2hlZExvYWRpbmdJY29uKEljb25Mb2FkZXImIGxvYWRlciwgU2hhcmVkQnVmZmVyKiBidWZm ZXIpCiB7CisgICAgLy8gSWYgdGhlIERvY3VtZW50TG9hZGVyIGhhcyBkZXRhY2hlZCBmcm9tIGl0 cyBmcmFtZSwgYWxsIGljb24gbG9hZHMgc2hvdWxkIGhhdmUgYWxyZWFkeSBiZWVuIGNhbmNlbGxl ZC4KKyAgICBBU1NFUlQobV9mcmFtZSk7CisKICAgICBhdXRvIGxvYWRJZGVudGlmaWVyID0gbV9p Y29uTG9hZGVycy50YWtlKCZsb2FkZXIpOwogICAgIEFTU0VSVChsb2FkSWRlbnRpZmllcik7CiAK