A number of crash fixes were done to prevent FormState objects from being accessed after their relevant Frames had been destroyed. Unfortunately, this could cause the FormState to persist after the owning Frame had been destroyed, resulting in nullptr dereferences. This patch does the following: 1. Changes to use WeakPtr's for FormState objects passed to completion handlers, rather than RetainPtr, since those completion handlers might fire as part of the clean-up process during Frame destruction. This allows us to use the FormState if they are still valid, but gracefully handle cases where a form submission is cancelled in-flight. 2. Removes some extraneous WTFMove() calls being made on bare FormState pointers. 3. Changes the trap from Bug 183704 so that it only fires if the FormState object is being retained more than once.
<rdar://problem/39329219>
Created attachment 341006 [details] Patch
Created attachment 341013 [details] Patch
Comment on attachment 341006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341006&action=review > Source/WebCore/ChangeLog:17 > + rather than RetainPtr, since those completion handlers might fire as part of You mean RefPtr. RetainPtr is for objective-C.
Created attachment 341019 [details] Patch for landing
Created attachment 341023 [details] Patch for landing
Comment on attachment 341023 [details] Patch for landing Clearing flags on attachment: 341023 Committed r232081: <https://trac.webkit.org/changeset/232081>
All reviewed patches have been landed. Closing bug.
I've observed 2 API test failures after this patch was landed. https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/5530/steps/run-api-tests/logs/stdio Failed TestWebKitAPI.WebKit.FormSubmission /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKInputDelegate.mm:53 Value of: willSubmitFormValuesCalled Actual: false Expected: true Timeout TestWebKitAPI.WebKit.DuplicateCompletionHandlerCalls
Comment on attachment 341023 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=341023&action=review > Source/WebCore/loader/PolicyChecker.cpp:171 > + m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, didReceiveRedirectResponse, formState, policyDecisionMode, [this, function = WTFMove(function), request = ResourceRequest(request), formState = makeWeakPtr(formState), suggestedFilename = WTFMove(suggestedFilename), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension)](PolicyAction policyAction) mutable { It seems wrong to be that we could loose the FormState during the async policy decision now. We initiated the navigation with a FormState, but then, after the client tells us to go for it, we may not end up using that FormState.
Re-opened since this is blocked by bug 185895
Created attachment 341070 [details] Patch
Created attachment 341166 [details] Patch
Created attachment 341170 [details] Patch for landing
Comment on attachment 341170 [details] Patch for landing Clearing flags on attachment: 341170 Committed r232147: <https://trac.webkit.org/changeset/232147>