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+

Brent Fulgham
Reported 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.
Attachments
Patch (3.22 KB, patch)
2018-06-25 11:04 PDT, Brent Fulgham 		cdumez: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Brent Fulgham
Comment 1 2018-06-25 11:04:14 PDT
Created attachment 343516 [details] Patch
Radar WebKit Bug Importer
Comment 2 2018-06-25 11:05:10 PDT
<rdar://problem/41430690>
Radar WebKit Bug Importer
Comment 3 2018-06-25 11:05:14 PDT
<rdar://problem/41430650>
Chris Dumez
Comment 4 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?
Brent Fulgham
Comment 5 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).
Brent Fulgham
Comment 6 2018-06-25 14:30:32 PDT
Committed r233176: <https://trac.webkit.org/changeset/233176>
Note You need to log in before you can comment on or make changes to this bug.