Bug 211544 - Add some missing null checks for DocumentLoader
Summary: Add some missing null checks for DocumentLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-06 19:00 PDT by Darin Adler
Modified: 2020-05-07 11:56 PDT (History)
6 users (show)

See Also:

Attachments
Patch (5.58 KB, patch)
2020-05-06 19:02 PDT, Darin Adler 		andersca: review+
Details | Formatted Diff | Diff
Patch (7.40 KB, patch)
2020-05-07 10:07 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (7.39 KB, patch)
2020-05-07 10:10 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-05-06 19:00:55 PDT 
Add some missing null checks for DocumentLoader
Comment 1 Darin Adler 2020-05-06 19:02:14 PDT 
Created attachment 398696 [details]
Patch
Comment 2 Darin Adler 2020-05-06 19:03:04 PDT 
rdar://62843516
Comment 3 Anders Carlsson 2020-05-07 07:33:15 PDT 
Comment on attachment 398696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398696&action=review

> Source/WebCore/loader/FrameLoader.cpp:2173
> +    auto dl = m_documentLoader;

I would have renamed this to documentLoader.

> Source/WebCore/loader/FrameLoader.cpp:2226
> +        m_documentLoader->writer().setMIMEType(dl->responseMIMEType());

Should this use dl instead of m_documentLoader? (Maybe there's a chance that m_documentLoader is replaced mid-function?)

> Source/WebCore/loader/HistoryController.cpp:417
> +    const URL& historyURL = m_frame.loader().documentLoader() ? m_frame.loader().documentLoader()->urlForHistory() : URL { };

I'd change this to no longer hold a const reference, since urlForHistory() returns a const&, it will always be copied anyway AFAICT.
Comment 4 Darin Adler 2020-05-07 08:44:24 PDT 
Comment on attachment 398696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398696&action=review

>> Source/WebCore/loader/FrameLoader.cpp:2173
>> +    auto dl = m_documentLoader;
> 
> I would have renamed this to documentLoader.

OK, I’ll do that.

>> Source/WebCore/loader/FrameLoader.cpp:2226
>> +        m_documentLoader->writer().setMIMEType(dl->responseMIMEType());
> 
> Should this use dl instead of m_documentLoader? (Maybe there's a chance that m_documentLoader is replaced mid-function?)

I thought the point was to set the MIME type on the new writer with the possibly-old loader’s MIME type. Not really sure.

>> Source/WebCore/loader/HistoryController.cpp:417
>> +    const URL& historyURL = m_frame.loader().documentLoader() ? m_frame.loader().documentLoader()->urlForHistory() : URL { };
> 
> I'd change this to no longer hold a const reference, since urlForHistory() returns a const&, it will always be copied anyway AFAICT.

Will do.
Comment 5 Darin Adler 2020-05-07 10:07:24 PDT Comment hidden (obsolete)
Comment 6 Darin Adler 2020-05-07 10:07:46 PDT Comment hidden (obsolete)
Comment 7 Darin Adler 2020-05-07 10:10:29 PDT 
Created attachment 398752 [details]
Patch
Comment 8 EWS 2020-05-07 11:55:55 PDT 
Committed r261323: <https://trac.webkit.org/changeset/261323>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398752 [details].
Comment 9 Radar WebKit Bug Importer 2020-05-07 11:56:15 PDT 
<rdar://problem/62986860>