Bug 187008

Summary: REGRESSION(r229722): WebKitLegacy clients can crash when loading alternate page
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit2Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, dbates, ews-watchlist, japhet, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 183702    
Bug Blocks: 187121    
Attachments:
Description Flags
Patch cdumez: review+

Description Brent Fulgham 2018-06-25 11:00:14 PDT 
The new call to 'clearProvisionalLoadForPolicyCheck' added in r229722 broke loading behavior in WebKitLegacy.

1. We can now enter 'cancelPolicyCheckIfNeeded' without a Frame loader, in what appears to be a recursive call during the load cancellation (the 'm_waitingForContentPolicy' and 'm_waitingForNavigationPolicy' have already been nulled). It seems like we should return early here, or perhaps just move the RELEASE_ASSERT inside the case where we have an active policy check happening.

2. We also enter FrameLoader::checkContentPolicy without an active document loader. We should recognize this case and handle it, rather than trying to dereference a nullptr document loader.
Comment 1 Brent Fulgham 2018-06-25 11:04:14 PDT 
Created attachment 343516 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2018-06-25 11:05:10 PDT 
<rdar://problem/41430690>
Comment 3 Radar WebKit Bug Importer 2018-06-25 11:05:14 PDT 
<rdar://problem/41430650>
Comment 4 Chris Dumez 2018-06-25 12:32:59 PDT 
Comment on attachment 343516 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:363
>  void FrameLoader::checkContentPolicy(const ResourceResponse& response, ContentPolicyDecisionFunction&& function)

The crash traces attached to the radar do not seem to involve FrameLoader::checkContentPolicy(), could you clarify why this change is needed?
Comment 5 Brent Fulgham 2018-06-25 13:16:17 PDT 
Comment on attachment 343516 [details]
Patch

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

>> Source/WebCore/loader/FrameLoader.cpp:363
>>  void FrameLoader::checkContentPolicy(const ResourceResponse& response, ContentPolicyDecisionFunction&& function)
> 
> The crash traces attached to the radar do not seem to involve FrameLoader::checkContentPolicy(), could you clarify why this change is needed?

Yes, sorry -- this code is hit once you clear the RELEASE_ASSERT from DocumentLoader.cpp.

(Historical Note: I went and spoke with Chris in person about the issue before he completed the review).
Comment 6 Brent Fulgham 2018-06-25 14:30:32 PDT 
Committed r233176: <https://trac.webkit.org/changeset/233176>