Changeset 232081 in webkit


Ignore:
Timestamp:
May 22, 2018 3:01:34 PM (6 years ago)
Author:
Brent Fulgham
Message:

Avoid keeping FormState alive longer than necessary
​https://bugs.webkit.org/show_bug.cgi?id=185877
<rdar://problem/39329219>

Reviewed by Ryosuke Niwa.

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 RefPtr, 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.
  • loader/DocumentLoader.cpp:

(WebCore::DocumentLoader::willSendRequest): Update for new CompletionHandler
signature.

  • loader/FormState.cpp:

(WebCore::FormState::willDetachPage): Revise trap to check for retain counts
above one.

  • loader/FormState.h:

(WebCore::FormState::weakPtrFactory const): Added.

  • loader/FrameLoader.cpp:

(WebCore::FrameLoader::loadFrameRequest): Revise to use WeakPtr for FormState
passed to the completion handler.
(WebCore::FrameLoader::loadURL): Update for new CompletionHandler signature.
(WebCore::FrameLoader::load): Ditto.
(WebCore::FrameLoader::loadWithDocumentLoader): Ditto.
(WebCore::FrameLoader::loadPostRequest): Ditto.

  • loader/PolicyChecker.cpp:

(WebCore::PolicyChecker::checkNavigationPolicy): Revise to use WeakPtr for
FormState passed to the completion handler. Remove some extraneous WTFMove()
calls on bare pointers.
(WebCore::PolicyChecker::checkNewWindowPolicy): Ditto.

  • loader/PolicyChecker.h:
Location:
trunk/Source/WebCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r232079 r232081  
     12018-05-22  Brent Fulgham  <bfulgham@apple.com>
     2
     3        Avoid keeping FormState alive longer than necessary
     4        https://bugs.webkit.org/show_bug.cgi?id=185877
     5        <rdar://problem/39329219>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        A number of crash fixes were done to prevent FormState objects from being
     10        accessed after their relevant Frames had been destroyed. Unfortunately, this
     11        could cause the FormState to persist after the owning Frame had been
     12        destroyed, resulting in nullptr dereferences.
     13
     14        This patch does the following:
     15
     16        1. Changes to use WeakPtr's for FormState objects passed to completion handlers,
     17           rather than RefPtr, since those completion handlers might fire as part of
     18           the clean-up process during Frame destruction. This allows us to use the FormState
     19           if they are still valid, but gracefully handle cases where a form submission
     20           is cancelled in-flight.
     21        2. Removes some extraneous WTFMove() calls being made on bare FormState pointers.
     22        3. Changes the trap from Bug 183704 so that it only fires if the FormState object
     23           is being retained more than once.
     24
     25        * loader/DocumentLoader.cpp:
     26        (WebCore::DocumentLoader::willSendRequest): Update for new CompletionHandler
     27        signature.
     28        * loader/FormState.cpp:
     29        (WebCore::FormState::willDetachPage): Revise trap to check for retain counts
     30        above one.
     31        * loader/FormState.h:
     32        (WebCore::FormState::weakPtrFactory const): Added.
     33        * loader/FrameLoader.cpp:
     34        (WebCore::FrameLoader::loadFrameRequest): Revise to use WeakPtr for FormState
     35        passed to the completion handler.
     36        (WebCore::FrameLoader::loadURL): Update for new CompletionHandler signature.
     37        (WebCore::FrameLoader::load): Ditto.
     38        (WebCore::FrameLoader::loadWithDocumentLoader): Ditto.
     39        (WebCore::FrameLoader::loadPostRequest): Ditto.
     40        * loader/PolicyChecker.cpp:
     41        (WebCore::PolicyChecker::checkNavigationPolicy): Revise to use WeakPtr for
     42        FormState passed to the completion handler. Remove some extraneous WTFMove()
     43        calls on bare pointers.
     44        (WebCore::PolicyChecker::checkNewWindowPolicy): Ditto.
     45        * loader/PolicyChecker.h:
     46
    1472018-05-22  Sihui Liu  <sihui_liu@apple.com>
    248

  • trunk/Source/WebCore/loader/DocumentLoader.cpp

    r232032 r232081  
    11/*
    2  * Copyright (C) 2006-2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
    33 * Copyright (C) 2011 Google Inc. All rights reserved.
    44 *
    … …  
    641641        return completionHandler(WTFMove(newRequest));
    642642
    643     auto navigationPolicyCompletionHandler = [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request, FormState*, ShouldContinue shouldContinue) mutable {
     643    auto navigationPolicyCompletionHandler = [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (ResourceRequest&& request, WeakPtr<FormState>&&, ShouldContinue shouldContinue) mutable {
    644644        m_waitingForNavigationPolicy = false;
    645645        switch (shouldContinue) {

  • trunk/Source/WebCore/loader/FormState.cpp

    r229683 r232081  
    11/*
    2  * Copyright (C) 2006-2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    5353{
    5454    // Beartrap for <rdar://problem/37579354>
    55     RELEASE_ASSERT_NOT_REACHED();
     55    RELEASE_ASSERT(hasOneRef());
    5656}
    5757

  • trunk/Source/WebCore/loader/FormState.h

    r229683 r232081  
    11/*
    2  * Copyright (C) 2006-2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    3030
    3131#include "FrameDestructionObserver.h"
     32#include <wtf/WeakPtr.h>
    3233#include <wtf/text/WTFString.h>
    3334
    … …  
    5051    FormSubmissionTrigger formSubmissionTrigger() const { return m_formSubmissionTrigger; }
    5152
     53    auto& weakPtrFactory() const { return m_weakFactory; }
     54
    5255private:
    5356    FormState(HTMLFormElement&, StringPairVector&& textFieldValues, Document&, FormSubmissionTrigger);
    … …  
    5861    Ref<Document> m_sourceDocument;
    5962    FormSubmissionTrigger m_formSubmissionTrigger;
     63    WeakPtrFactory<FormState> m_weakFactory;
    6064};
    6165

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r232019 r232081  
    11/*
    2  * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
    33 * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
    44 * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
    … …  
    12291229        loadType = FrameLoadType::Standard;
    12301230
    1231     auto completionHandler = [this, protectedFrame = makeRef(m_frame), formState = makeRefPtr(formState), frameName = request.frameName()] {
     1231    auto completionHandler = [this, protectedFrame = makeRef(m_frame), formState = makeWeakPtr(formState), frameName = request.frameName()] {
    12321232        // FIXME: It's possible this targetFrame will not be the same frame that was targeted by the actual
    12331233        // load if frame names have changed.
    … …  
    13391339    if (!targetFrame && !frameName.isEmpty()) {
    13401340        action = action.copyWithShouldOpenExternalURLsPolicy(shouldOpenExternalURLsPolicyToApply(m_frame, frameLoadRequest));
    1341         policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request), formState, frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
    1342             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
     1341        policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request), formState, frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
     1342            continueLoadAfterNewWindowPolicy(request, formState.get(), frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
    13431343            completionHandler();
    13441344        });
    … …  
    13591359        policyChecker().stopCheck();
    13601360        policyChecker().setLoadType(newLoadType);
    1361         policyChecker().checkNavigationPolicy(WTFMove(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
     1361        policyChecker().checkNavigationPolicy(WTFMove(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, WeakPtr<FormState>&&, ShouldContinue shouldContinue) {
    13621362            continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
    13631363        }, PolicyDecisionMode::Synchronous);
    … …  
    14151415    if (request.shouldCheckNewWindowPolicy()) {
    14161416        NavigationAction action { request.requester(), request.resourceRequest(), InitiatedByMainFrame::Unknown, NavigationType::Other, request.shouldOpenExternalURLsPolicy() };
    1417         policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request.resourceRequest()), nullptr, request.frameName(), [this] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
    1418             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, AllowNavigationToInvalidURL::Yes, NewFrameOpenerPolicy::Suppress);
     1417        policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request.resourceRequest()), nullptr, request.frameName(), [this] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
     1418            continueLoadAfterNewWindowPolicy(request, formState.get(), frameName, action, shouldContinue, AllowNavigationToInvalidURL::Yes, NewFrameOpenerPolicy::Suppress);
    14191419        });
    14201420
    … …  
    15291529        oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
    15301530        policyChecker().stopCheck();
    1531         policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
     1531        policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, WeakPtr<FormState>&&, ShouldContinue shouldContinue) {
    15321532            continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
    15331533        }, PolicyDecisionMode::Synchronous);
    … …  
    15631563    }
    15641564
    1565     policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, loader, formState, [this, protectedFrame = makeRef(m_frame), allowNavigationToInvalidURL, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, ShouldContinue shouldContinue) {
    1566         continueLoadAfterNavigationPolicy(request, formState, shouldContinue, allowNavigationToInvalidURL);
     1565    policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, loader, formState, [this, protectedFrame = makeRef(m_frame), allowNavigationToInvalidURL, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, WeakPtr<FormState>&& formState, ShouldContinue shouldContinue) {
     1566        continueLoadAfterNavigationPolicy(request, formState.get(), shouldContinue, allowNavigationToInvalidURL);
    15671567        completionHandler();
    15681568    });
    … …  
    28572857    if (!frameName.isEmpty()) {
    28582858        // The search for a target frame is done earlier in the case of form submission.
    2859         if (Frame* targetFrame = formState ? 0 : findFrameForNavigation(frameName)) {
    2860             targetFrame->loader().loadWithNavigationAction(workingResourceRequest, action, lockHistory, loadType, WTFMove(formState), allowNavigationToInvalidURL, WTFMove(completionHandler));
     2859        if (auto* targetFrame = formState ? nullptr : findFrameForNavigation(frameName)) {
     2860            targetFrame->loader().loadWithNavigationAction(workingResourceRequest, action, lockHistory, loadType, formState, allowNavigationToInvalidURL, WTFMove(completionHandler));
    28612861            return;
    28622862        }
    28632863
    2864         policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(workingResourceRequest), WTFMove(formState), frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = WTFMove(completionHandler)] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
    2865             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
     2864        policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(workingResourceRequest), formState, frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = WTFMove(completionHandler)] (const ResourceRequest& request, WeakPtr<FormState>&& formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
     2865            continueLoadAfterNewWindowPolicy(request, formState.get(), frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
    28662866            completionHandler();
    28672867        });
    … …  
    28712871    // must grab this now, since this load may stop the previous load and clear this flag
    28722872    bool isRedirect = m_quickRedirectComing;
    2873     loadWithNavigationAction(workingResourceRequest, action, lockHistory, loadType, WTFMove(formState), allowNavigationToInvalidURL, [this, isRedirect, protectedFrame = makeRef(m_frame), completionHandler = WTFMove(completionHandler)] {
     2873    loadWithNavigationAction(workingResourceRequest, action, lockHistory, loadType, formState, allowNavigationToInvalidURL, [this, isRedirect, protectedFrame = makeRef(m_frame), completionHandler = WTFMove(completionHandler)] {
    28742874        if (isRedirect) {
    28752875            m_quickRedirectComing = false;

  • trunk/Source/WebCore/loader/PolicyChecker.cpp

    r231813 r232081  
    11/*
    2  * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
    33 * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
    44 * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
    … …  
    110110    // This avoids confusion on the part of the client.
    111111    if (equalIgnoringHeaderFields(request, loader->lastCheckedRequest()) || (!request.isNull() && request.url().isEmpty())) {
    112         function(ResourceRequest(request), nullptr, ShouldContinue::Yes);
     112        function(ResourceRequest(request), { }, ShouldContinue::Yes);
    113113        loader->setLastCheckedRequest(WTFMove(request));
    114114        return;
    … …  
    125125        if (isBackForwardLoadType(m_loadType))
    126126            m_loadType = FrameLoadType::Reload;
    127         function(WTFMove(request), nullptr, shouldContinue ? ShouldContinue::Yes : ShouldContinue::No);
     127        function(WTFMove(request), { }, shouldContinue ? ShouldContinue::Yes : ShouldContinue::No);
    128128        return;
    129129    }
    … …  
    135135            m_frame.ownerElement()->dispatchEvent(Event::create(eventNames().loadEvent, false, false));
    136136        }
    137         function(WTFMove(request), nullptr, ShouldContinue::No);
     137        function(WTFMove(request), { }, ShouldContinue::No);
    138138        return;
    139139    }
    … …  
    148148    // Always allow QuickLook-generated URLs based on the protocol scheme.
    149149    if (!request.isNull() && isQuickLookPreviewURL(request.url()))
    150         return function(WTFMove(request), formState, ShouldContinue::Yes);
     150        return function(WTFMove(request), makeWeakPtr(formState), ShouldContinue::Yes);
    151151#endif
    152152
    … …  
    169169    m_delegateIsDecidingNavigationPolicy = true;
    170170    String suggestedFilename = action.downloadAttribute().isEmpty() ? nullAtom() : action.downloadAttribute();
    171     m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, didReceiveRedirectResponse, formState, policyDecisionMode, [this, function = WTFMove(function), request = ResourceRequest(request), formState = makeRefPtr(formState), suggestedFilename = WTFMove(suggestedFilename), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension)](PolicyAction policyAction) mutable {
     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 {
    172172        m_delegateIsDecidingNavigationPolicy = false;
    173173
    … …  
    186186                return function({ }, nullptr, ShouldContinue::No);
    187187            }
    188             return function(WTFMove(request), formState.get(), ShouldContinue::Yes);
     188            return function(WTFMove(request), WTFMove(formState), ShouldContinue::Yes);
    189189        }
    190190        ASSERT_NOT_REACHED();
    … …  
    202202    auto blobURLLifetimeExtension = extendBlobURLLifetimeIfNecessary(request);
    203203
    204     m_frame.loader().client().dispatchDecidePolicyForNewWindowAction(navigationAction, request, formState, frameName, [frame = makeRef(m_frame), request, formState = makeRefPtr(formState), frameName, navigationAction, function = WTFMove(function), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension)](PolicyAction policyAction) mutable {
     204    m_frame.loader().client().dispatchDecidePolicyForNewWindowAction(navigationAction, request, formState, frameName, [frame = makeRef(m_frame), request, formState = makeWeakPtr(formState), frameName, navigationAction, function = WTFMove(function), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension)](PolicyAction policyAction) mutable {
    205205        switch (policyAction) {
    206206        case PolicyAction::Download:
    … …  
    214214            RELEASE_ASSERT_NOT_REACHED();
    215215        case PolicyAction::Use:
    216             function(request, formState.get(), frameName, navigationAction, ShouldContinue::Yes);
     216            function(request, WTFMove(formState), frameName, navigationAction, ShouldContinue::Yes);
    217217            return;
    218218        }

  • trunk/Source/WebCore/loader/PolicyChecker.h

    r231714 r232081  
    11/*
    2  * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
    33 * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
    44 *
    … …  
    3232#include "FrameLoaderTypes.h"
    3333#include "ResourceRequest.h"
     34#include <wtf/WeakPtr.h>
    3435#include <wtf/text/WTFString.h>
    3536
    … …  
    6061enum class PolicyDecisionMode { Synchronous, Asynchronous };
    6162
    62 using NewWindowPolicyDecisionFunction = CompletionHandler<void(const ResourceRequest&, FormState*, const String& frameName, const NavigationAction&, ShouldContinue)>;
    63 using NavigationPolicyDecisionFunction = CompletionHandler<void(ResourceRequest&&, FormState*, ShouldContinue)>;
     63using NewWindowPolicyDecisionFunction = CompletionHandler<void(const ResourceRequest&, WeakPtr<FormState>&&, const String& frameName, const NavigationAction&, ShouldContinue)>;
     64using NavigationPolicyDecisionFunction = CompletionHandler<void(ResourceRequest&&, WeakPtr<FormState>&&, ShouldContinue)>;
    6465
    6566class PolicyChecker {
Note: See TracChangeset for help on using the changeset viewer.