Add missing null-check of page in ResourceLoader::loadDataURL
Created attachment 398774 [details] Patch
rdar://57213601
Comment on attachment 398774 [details] 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?
Comment on attachment 398774 [details] 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?
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.
And test results seem to confirm that.
(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.
(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.
(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?
Committed r261438: <https://trac.webkit.org/changeset/261438> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398774 [details].