211589 2020-05-07 12:10:26 -0700 Add missing null-check of page in ResourceLoader::loadDataURL 2020-05-09 09:07:54 -0700 1 1 1 Unclassified WebKit Page Loading WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 darin darin achristensen andersca beidson cdumez ews-watchlist japhet sam webkit-bug-importer oldest_to_newest 1650271 0 darin 2020-05-07 12:10:26 -0700 Add missing null-check of page in ResourceLoader::loadDataURL 1650272 1 398774 darin 2020-05-07 12:10:55 -0700 Created attachment 398774 Patch 1650273 2 darin 2020-05-07 12:11:23 -0700 rdar://57213601 1650284 3 398774 sam 2020-05-07 12:23:56 -0700 Comment on attachment 398774 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398774&action=review > Source/WebCore/loader/ResourceLoader.cpp:274 > + if (auto page = m_frame->page()) { > + if (auto scheduledPairs = page->scheduledRunLoopPairs()) > + scheduleContext.scheduledPairs = *scheduledPairs; > + } What's the effect to the rest of this function is page is null? When HAVE(RUNLOOP_TIMER) is enabled and you don't set scheduledPairs, does the call to DataURLDecoder::decode continue to work? 1650290 4 398774 darin 2020-05-07 12:32:08 -0700 Comment on attachment 398774 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398774&action=review >> Source/WebCore/loader/ResourceLoader.cpp:274 >> + } > > What's the effect to the rest of this function is page is null? When HAVE(RUNLOOP_TIMER) is enabled and you don't set scheduledPairs, does the call to DataURLDecoder::decode continue to work? Three thoughts: 1) This mostly happens when scheduling a load during tear-down, and so I think the load never completes and gets torn down. 2) Another way to ask this is to ask what happens to all the scheduled stuff when the frame detaches from the page. 3) Is all of this for legacy WebKit on Cocoa platforms only? 1650296 5 darin 2020-05-07 12:45:03 -0700 I suspect the real answer is (1) above. This is just one of multiple ways to harmlessly do nothing. Could also return and do nothing harmlessly. 1650299 6 darin 2020-05-07 12:45:43 -0700 And test results seem to confirm that. 1650497 7 achristensen 2020-05-07 21:51:45 -0700 (In reply to Darin Adler from comment #4) > 3) Is all of this for legacy WebKit on Cocoa platforms only? Yes. While we're at it, why don't we null check the frame? I see that null checked elsewhere in this file. 1650614 8 darin 2020-05-08 09:05:20 -0700 (In reply to Alex Christensen from comment #7) > While we're at it, why don't we null check the frame? I see that null > checked elsewhere in this file. Seems fine to do that. Good to do it more consistently. 1650913 9 darin 2020-05-09 06:36:16 -0700 (In reply to Darin Adler from comment #8) > Seems fine to do that. Good to do it more consistently. But can I do that in a separate patch? 1650929 10 ews-feeder 2020-05-09 09:07:53 -0700 Committed r261438: <https://trac.webkit.org/changeset/261438> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398774. 398774 2020-05-07 12:10:55 -0700 2020-05-09 09:07:53 -0700 Patch bug-211589-20200507121055.patch text/plain 1640 darin U3VidmVyc2lvbiBSZXZpc2lvbjogMjYxMzIzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMzJmOTM1N2M3MGNlZTc0 NzQ3OWQzZDVjYmY4ZTU5MTgzNzQyNmU3Yy4uZjE1MjMwNWQ2ZWI5ZGFlOGM4MTBhMDcyODMyYzM0 NjUzZjc1NzQ3OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDIwLTA1LTA3ICBEYXJp biBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KKworICAgICAgICBBZGQgbWlzc2luZyBudWxsLWNo ZWNrIG9mIHBhZ2UgaW4gUmVzb3VyY2VMb2FkZXI6OmxvYWREYXRhVVJMCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTE1ODkKKyAgICAgICAgcmRhcjov LzU3MjEzNjAxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgKiBsb2FkZXIvUmVzb3VyY2VMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVzb3Vy Y2VMb2FkZXI6OmxvYWREYXRhVVJMKTogQWRkIG51bGwgY2hlY2sgYmVmb3JlIHVzaW5nIHBhZ2Uu CisKIDIwMjAtMDUtMDcgIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29tPgogCiAgICAgICAg IEFkZCBzb21lIG1pc3NpbmcgbnVsbCBjaGVja3MgZm9yIERvY3VtZW50TG9hZGVyCmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvUmVzb3VyY2VMb2FkZXIuY3BwIGIvU291cmNlL1dl YkNvcmUvbG9hZGVyL1Jlc291cmNlTG9hZGVyLmNwcAppbmRleCA0NDVjZDNmY2VlMmI0NjQ2MjEy Zjk2NDU2ODg3NzQyMWRmYjc4MDNmLi5hM2YyZGU4Yjk5ODNjMmM3Mjk5ZjVkOTBhN2I4ZTM1M2Ex NjYzYjVlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvUmVzb3VyY2VMb2FkZXIu Y3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9SZXNvdXJjZUxvYWRlci5jcHAKQEAgLTI2 OCw4ICsyNjgsMTAgQEAgdm9pZCBSZXNvdXJjZUxvYWRlcjo6bG9hZERhdGFVUkwoKQogCiAgICAg RGF0YVVSTERlY29kZXI6OlNjaGVkdWxlQ29udGV4dCBzY2hlZHVsZUNvbnRleHQ7CiAjaWYgSEFW RShSVU5MT09QX1RJTUVSKQotICAgIGlmIChhdXRvKiBzY2hlZHVsZWRQYWlycyA9IG1fZnJhbWUt PnBhZ2UoKS0+c2NoZWR1bGVkUnVuTG9vcFBhaXJzKCkpCi0gICAgICAgIHNjaGVkdWxlQ29udGV4 dC5zY2hlZHVsZWRQYWlycyA9ICpzY2hlZHVsZWRQYWlyczsKKyAgICBpZiAoYXV0byBwYWdlID0g bV9mcmFtZS0+cGFnZSgpKSB7CisgICAgICAgIGlmIChhdXRvIHNjaGVkdWxlZFBhaXJzID0gcGFn ZS0+c2NoZWR1bGVkUnVuTG9vcFBhaXJzKCkpCisgICAgICAgICAgICBzY2hlZHVsZUNvbnRleHQu c2NoZWR1bGVkUGFpcnMgPSAqc2NoZWR1bGVkUGFpcnM7CisgICAgfQogI2VuZGlmCiAgICAgRGF0 YVVSTERlY29kZXI6OmRlY29kZSh1cmwsIHNjaGVkdWxlQ29udGV4dCwgW3RoaXMsIHByb3RlY3Rl ZFRoaXMgPSBtYWtlUmVmKCp0aGlzKSwgdXJsXShhdXRvIGRlY29kZVJlc3VsdCkgbXV0YWJsZSB7 CiAgICAgICAgIGlmICh0aGlzLT5yZWFjaGVkVGVybWluYWxTdGF0ZSgpKQo=