Summary: | Add some missing null checks for DocumentLoader | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
Component: | Page Loading | Assignee: | Darin Adler <darin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, beidson, cdumez, ews-watchlist, japhet, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Darin Adler
2020-05-06 19:00:55 PDT
Created attachment 398696 [details]
Patch
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 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. Created attachment 398751 [details]
Patch
Comment on attachment 398751 [details]
Patch
I plan to land this later with the commit queue.
Created attachment 398752 [details]
Patch
Committed r261323: <https://trac.webkit.org/changeset/261323> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398752 [details]. |