Bug 211589 – Add missing null-check of page in ResourceLoader::loadDataURL

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Adler]

Add missing null-check of page in ResourceLoader::loadDataURL

[Darin Adler]

Created attachment 398774 [details]
Patch

[Darin Adler]

rdar://57213601

[Sam Weinig]

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?

[Darin Adler]

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?

[Darin Adler]

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.

[Darin Adler]

And test results seem to confirm that.

[Alex Christensen]

(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.

[Darin Adler]

(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.

[Darin Adler]

(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?

[EWS]

Committed r261438: <https://trac.webkit.org/changeset/261438>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398774 [details].