Bug 185877

Summary: Avoid keeping FormState alive longer than necessary
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Severity: Normal CC: cdumez, commit-queue, dbates, ews-watchlist, japhet, realdawei, rniwa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 185895    
Bug Blocks:    
Description Flags
Patch for landing
Patch for landing
Patch for landing none

Description Brent Fulgham 2018-05-22 12:25:45 PDT
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.
Comment 1 Brent Fulgham 2018-05-22 12:26:35 PDT
Comment 2 Brent Fulgham 2018-05-22 12:31:31 PDT
Created attachment 341006 [details]
Comment 3 Brent Fulgham 2018-05-22 13:44:19 PDT
Created attachment 341013 [details]
Comment 4 Ryosuke Niwa 2018-05-22 13:45:00 PDT
Comment on attachment 341006 [details]

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.
Comment 5 Brent Fulgham 2018-05-22 14:03:15 PDT
Created attachment 341019 [details]
Patch for landing
Comment 6 Brent Fulgham 2018-05-22 14:16:12 PDT
Created attachment 341023 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2018-05-22 15:01:40 PDT
Comment on attachment 341023 [details]
Patch for landing

Clearing flags on attachment: 341023

Committed r232081: <https://trac.webkit.org/changeset/232081>
Comment 8 WebKit Commit Bot 2018-05-22 15:01:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Dawei Fenton (:realdawei) 2018-05-22 16:47:47 PDT
I've observed 2 API test failures after this patch was landed.



        Value of: willSubmitFormValuesCalled
          Actual: false
        Expected: true


Comment 10 Chris Dumez 2018-05-22 16:57:25 PDT
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.
Comment 11 WebKit Commit Bot 2018-05-22 17:16:19 PDT
Re-opened since this is blocked by bug 185895
Comment 12 Brent Fulgham 2018-05-22 21:54:44 PDT
Created attachment 341070 [details]
Comment 13 Brent Fulgham 2018-05-23 20:36:10 PDT
Created attachment 341166 [details]
Comment 14 Brent Fulgham 2018-05-23 21:44:09 PDT
Created attachment 341170 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2018-05-23 22:23:05 PDT
Comment on attachment 341170 [details]
Patch for landing

Clearing flags on attachment: 341170

Committed r232147: <https://trac.webkit.org/changeset/232147>
Comment 16 WebKit Commit Bot 2018-05-23 22:23:07 PDT
All reviewed patches have been landed.  Closing bug.