Bug 185877 – Avoid keeping FormState alive longer than necessary

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/39329219>

[Brent Fulgham]

Created attachment 341006 [details]
Patch

[Brent Fulgham]

Created attachment 341013 [details]
Patch

[Ryosuke Niwa]

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.

[Brent Fulgham]

Created attachment 341019 [details]
Patch for landing

[Brent Fulgham]

Created attachment 341023 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 341023 [details]
Patch for landing

Clearing flags on attachment: 341023

Committed r232081: <https://trac.webkit.org/changeset/232081>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dawei Fenton (:realdawei)]

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

[Chris Dumez]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185895

[Brent Fulgham]

Created attachment 341070 [details]
Patch

[Brent Fulgham]

Created attachment 341166 [details]
Patch

[Brent Fulgham]

Created attachment 341170 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 341170 [details]
Patch for landing

Clearing flags on attachment: 341170

Committed r232147: <https://trac.webkit.org/changeset/232147>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.