Make WebPaymentCoordinatorProxy more robust and modern
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2019 21:25:28 +0000 (21:25 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Feb 2019 21:25:28 +0000 (21:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194678

Reviewed by Andy Estes.

Use WeakPtr instead of storing raw pointers in lambdas or the global activePaymentCoordinatorProxy to avoid UAF problems.
Call CompletionHandlers in all code paths to avoid hangs.
Use Delayed instead of LegacySync for synchronous messaging to progress towards removing LegacySync messages.

* Scripts/webkit/messages.py:
* UIProcess/ApplePay/WebPaymentCoordinatorProxy.cpp:
(WebKit::activePaymentCoordinatorProxy):
(WebKit::WebPaymentCoordinatorProxy::~WebPaymentCoordinatorProxy):
(WebKit::WebPaymentCoordinatorProxy::availablePaymentNetworks):
(WebKit::WebPaymentCoordinatorProxy::canMakePayments):
(WebKit::WebPaymentCoordinatorProxy::showPaymentUI):
(WebKit::WebPaymentCoordinatorProxy::didReachFinalState):
* UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:
* UIProcess/ApplePay/WebPaymentCoordinatorProxy.messages.in:
* UIProcess/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm:
(WebKit::WebPaymentCoordinatorProxy::platformShowPaymentUI):
* UIProcess/ApplePay/mac/WebPaymentCoordinatorProxyMac.mm:
(WebKit::WebPaymentCoordinatorProxy::platformShowPaymentUI):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@241611 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Scripts/webkit/messages.py
Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.cpp
Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h
Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.messages.in
Source/WebKit/UIProcess/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm
Source/WebKit/UIProcess/ApplePay/mac/WebPaymentCoordinatorProxyMac.mm

index 33b3b2c..719e980 100644 (file)
@@ -1,3 +1,29 @@
+2019-02-15  Alex Christensen  <achristensen@webkit.org>
+
+        Make WebPaymentCoordinatorProxy more robust and modern
+        https://bugs.webkit.org/show_bug.cgi?id=194678
+
+        Reviewed by Andy Estes.
+
+        Use WeakPtr instead of storing raw pointers in lambdas or the global activePaymentCoordinatorProxy to avoid UAF problems.
+        Call CompletionHandlers in all code paths to avoid hangs.
+        Use Delayed instead of LegacySync for synchronous messaging to progress towards removing LegacySync messages.
+
+        * Scripts/webkit/messages.py:
+        * UIProcess/ApplePay/WebPaymentCoordinatorProxy.cpp:
+        (WebKit::activePaymentCoordinatorProxy):
+        (WebKit::WebPaymentCoordinatorProxy::~WebPaymentCoordinatorProxy):
+        (WebKit::WebPaymentCoordinatorProxy::availablePaymentNetworks):
+        (WebKit::WebPaymentCoordinatorProxy::canMakePayments):
+        (WebKit::WebPaymentCoordinatorProxy::showPaymentUI):
+        (WebKit::WebPaymentCoordinatorProxy::didReachFinalState):
+        * UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:
+        * UIProcess/ApplePay/WebPaymentCoordinatorProxy.messages.in:
+        * UIProcess/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm:
+        (WebKit::WebPaymentCoordinatorProxy::platformShowPaymentUI):
+        * UIProcess/ApplePay/mac/WebPaymentCoordinatorProxyMac.mm:
+        (WebKit::WebPaymentCoordinatorProxy::platformShowPaymentUI):
+
 2019-02-15  Youenn Fablet  <youenn@apple.com>
 
         Make ServiceWorkerClientFetch closer to WebResourceLoader
index 9ae038a..6f61413 100644 (file)
@@ -189,10 +189,8 @@ def forward_declarations_and_headers(receiver):
         'String',
     ])
 
-    for message in receiver.messages:
-        if message.reply_parameters != None:
-            headers.add('<wtf/ThreadSafeRefCounted.h>')
-            types_by_namespace['IPC'].update([('class', 'Connection')])
+    headers.add('"Connection.h"')
+    headers.add('<wtf/ThreadSafeRefCounted.h>')
 
     no_forward_declaration_types = frozenset([
         'MachSendRight',
index 3edbb8f..428651f 100644 (file)
 
 namespace WebKit {
 
-static WebPaymentCoordinatorProxy* activePaymentCoordinatorProxy;
+static WeakPtr<WebPaymentCoordinatorProxy>& activePaymentCoordinatorProxy()
+{
+    static NeverDestroyed<WeakPtr<WebPaymentCoordinatorProxy>> activePaymentCoordinatorProxy;
+    return activePaymentCoordinatorProxy.get();
+}
 
 WebPaymentCoordinatorProxy::WebPaymentCoordinatorProxy(WebPageProxy& webPageProxy)
     : m_webPageProxy(webPageProxy)
@@ -49,23 +53,20 @@ WebPaymentCoordinatorProxy::WebPaymentCoordinatorProxy(WebPageProxy& webPageProx
 
 WebPaymentCoordinatorProxy::~WebPaymentCoordinatorProxy()
 {
-    if (activePaymentCoordinatorProxy == this)
-        activePaymentCoordinatorProxy = nullptr;
-
     if (m_state != State::Idle)
         hidePaymentUI();
 
     m_webPageProxy.process().removeMessageReceiver(Messages::WebPaymentCoordinatorProxy::messageReceiverName(), m_webPageProxy.pageID());
 }
 
-void WebPaymentCoordinatorProxy::availablePaymentNetworks(Vector<String>& networks)
+void WebPaymentCoordinatorProxy::availablePaymentNetworks(CompletionHandler<void(Vector<String>&&)>&& completionHandler)
 {
-    networks = platformAvailablePaymentNetworks();
+    completionHandler(platformAvailablePaymentNetworks());
 }
 
-void WebPaymentCoordinatorProxy::canMakePayments(bool& reply)
+void WebPaymentCoordinatorProxy::canMakePayments(CompletionHandler<void(bool)>&& reply)
 {
-    reply = platformCanMakePayments();
+    reply(platformCanMakePayments());
 }
 
 void WebPaymentCoordinatorProxy::canMakePaymentsWithActiveCard(const String& merchantIdentifier, const String& domainName, uint64_t requestID)
@@ -92,17 +93,17 @@ void WebPaymentCoordinatorProxy::openPaymentSetup(const String& merchantIdentifi
     });
 }
 
-void WebPaymentCoordinatorProxy::showPaymentUI(const String& originatingURLString, const Vector<String>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest& paymentRequest, bool& result)
+void WebPaymentCoordinatorProxy::showPaymentUI(const String& originatingURLString, const Vector<String>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest& paymentRequest, CompletionHandler<void(bool)>&& completionHandler)
 {
     // FIXME: Make this a message check.
     ASSERT(canBegin());
 
-    if (activePaymentCoordinatorProxy) {
-        activePaymentCoordinatorProxy->hidePaymentUI();
-        activePaymentCoordinatorProxy->didCancelPaymentSession();
+    if (auto& coordinator = activePaymentCoordinatorProxy()) {
+        coordinator->hidePaymentUI();
+        coordinator->didCancelPaymentSession();
     }
 
-    activePaymentCoordinatorProxy = this;
+    activePaymentCoordinatorProxy() = makeWeakPtr(this);
 
     m_state = State::Activating;
 
@@ -112,17 +113,20 @@ void WebPaymentCoordinatorProxy::showPaymentUI(const String& originatingURLStrin
     for (const auto& linkIconURLString : linkIconURLStrings)
         linkIconURLs.append(URL(URL(), linkIconURLString));
 
-    platformShowPaymentUI(originatingURL, linkIconURLs, paymentRequest, [this](bool result) {
-        ASSERT(m_state == State::Activating);
+    platformShowPaymentUI(originatingURL, linkIconURLs, paymentRequest, [weakThis = makeWeakPtr(*this)](bool result) {
+        if (!weakThis)
+            return;
+
+        ASSERT(weakThis->m_state == State::Activating);
         if (!result) {
-            didCancelPaymentSession();
+            weakThis->didCancelPaymentSession();
             return;
         }
 
-        m_state = State::Active;
+        weakThis->m_state = State::Active;
     });
 
-    result = true;
+    completionHandler(true);
 }
 
     
@@ -332,8 +336,8 @@ void WebPaymentCoordinatorProxy::didReachFinalState()
     m_state = State::Idle;
     m_merchantValidationState = MerchantValidationState::Idle;
 
-    ASSERT(activePaymentCoordinatorProxy == this);
-    activePaymentCoordinatorProxy = nullptr;
+    ASSERT(activePaymentCoordinatorProxy() == this);
+    activePaymentCoordinatorProxy() = nullptr;
 }
 
 }
index 745c8eb..e2da444 100644 (file)
@@ -78,11 +78,11 @@ private:
     void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&) override;
 
     // Message handlers.
-    void availablePaymentNetworks(Vector<String>&);
-    void canMakePayments(bool& reply);
+    void availablePaymentNetworks(CompletionHandler<void(Vector<String>&&)>&&);
+    void canMakePayments(CompletionHandler<void(bool)>&&);
     void canMakePaymentsWithActiveCard(const String& merchantIdentifier, const String& domainName, uint64_t requestID);
     void openPaymentSetup(const String& merchantIdentifier, const String& domainName, uint64_t requestID);
-    void showPaymentUI(const String& originatingURLString, const Vector<String>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest&, bool& result);
+    void showPaymentUI(const String& originatingURLString, const Vector<String>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest&, CompletionHandler<void(bool)>&&);
     void completeMerchantValidation(const WebCore::PaymentMerchantSession&);
     void completeShippingMethodSelection(const Optional<WebCore::ShippingMethodUpdate>&);
     void completeShippingContactSelection(const Optional<WebCore::ShippingContactUpdate>&);
@@ -102,7 +102,7 @@ private:
     bool platformCanMakePayments();
     void platformCanMakePaymentsWithActiveCard(const String& merchantIdentifier, const String& domainName, WTF::Function<void (bool)>&& completionHandler);
     void platformOpenPaymentSetup(const String& merchantIdentifier, const String& domainName, WTF::Function<void (bool)>&& completionHandler);
-    void platformShowPaymentUI(const URL& originatingURL, const Vector<URL>& linkIconURLs, const WebCore::ApplePaySessionPaymentRequest&, WTF::Function<void(bool)>&& completionHandler);
+    void platformShowPaymentUI(const URL& originatingURL, const Vector<URL>& linkIconURLs, const WebCore::ApplePaySessionPaymentRequest&, CompletionHandler<void(bool)>&&);
     void platformCompleteMerchantValidation(const WebCore::PaymentMerchantSession&);
     void platformCompleteShippingMethodSelection(const Optional<WebCore::ShippingMethodUpdate>&);
     void platformCompleteShippingContactSelection(const Optional<WebCore::ShippingContactUpdate>&);
index f60664d..d4446f5 100644 (file)
 
 messages -> WebPaymentCoordinatorProxy {
 
-    AvailablePaymentNetworks() -> (Vector<String> availablePaymentNetworks) LegacySync
-    CanMakePayments() -> (bool result) LegacySync
+    AvailablePaymentNetworks() -> (Vector<String> availablePaymentNetworks) Delayed
+    CanMakePayments() -> (bool result) Delayed
     CanMakePaymentsWithActiveCard(String merchantIdentifier, String domainName, uint64_t requestID)
     OpenPaymentSetup(String merchantIdentifier, String domainName, uint64_t requestID)
 
-    ShowPaymentUI(String originatingURLString, Vector<String> linkIconURLStrings, WebCore::ApplePaySessionPaymentRequest paymentRequest) -> (bool result) LegacySync
+    ShowPaymentUI(String originatingURLString, Vector<String> linkIconURLStrings, WebCore::ApplePaySessionPaymentRequest paymentRequest) -> (bool result) Delayed
     CompleteMerchantValidation(WebCore::PaymentMerchantSession paymentMerchantSession)
     CompleteShippingMethodSelection(Optional<WebCore::ShippingMethodUpdate> update)
     CompleteShippingContactSelection(Optional<WebCore::ShippingContactUpdate> update)
index 5d7b43e..36b4bfe 100644 (file)
@@ -38,7 +38,7 @@
 
 namespace WebKit {
 
-void WebPaymentCoordinatorProxy::platformShowPaymentUI(const URL& originatingURL, const Vector<URL>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest& request, WTF::Function<void(bool)>&& completionHandler)
+void WebPaymentCoordinatorProxy::platformShowPaymentUI(const URL& originatingURL, const Vector<URL>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest& request, CompletionHandler<void(bool)>&& completionHandler)
 {
     UIViewController *presentingViewController = m_webPageProxy.uiClient().presentingViewController();
 
index 37897f2..e2601ff 100644 (file)
 
 namespace WebKit {
 
-void WebPaymentCoordinatorProxy::platformShowPaymentUI(const URL& originatingURL, const Vector<URL>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest& request, WTF::Function<void(bool)>&& completionHandler)
+void WebPaymentCoordinatorProxy::platformShowPaymentUI(const URL& originatingURL, const Vector<URL>& linkIconURLStrings, const WebCore::ApplePaySessionPaymentRequest& request, CompletionHandler<void(bool)>&& completionHandler)
 {
     auto paymentRequest = toPKPaymentRequest(m_webPageProxy, originatingURL, linkIconURLStrings, request);
 
     auto showPaymentUIRequestSeed = m_showPaymentUIRequestSeed;
     auto weakThis = makeWeakPtr(*this);
-    [PAL::getPKPaymentAuthorizationViewControllerClass() requestViewControllerWithPaymentRequest:paymentRequest.get() completion:makeBlockPtr([paymentRequest, showPaymentUIRequestSeed, weakThis, completionHandler = WTFMove(completionHandler)](PKPaymentAuthorizationViewController *viewController, NSError *error) {
+    [PAL::getPKPaymentAuthorizationViewControllerClass() requestViewControllerWithPaymentRequest:paymentRequest.get() completion:makeBlockPtr([paymentRequest, showPaymentUIRequestSeed, weakThis, completionHandler = WTFMove(completionHandler)](PKPaymentAuthorizationViewController *viewController, NSError *error) mutable {
         auto paymentCoordinatorProxy = weakThis.get();
         if (!paymentCoordinatorProxy)
-            return;
+            return completionHandler(false);
 
         if (error) {
             LOG_ERROR("+[PKPaymentAuthorizationViewController requestViewControllerWithPaymentRequest:completion:] error %@", error);
@@ -55,7 +55,7 @@ void WebPaymentCoordinatorProxy::platformShowPaymentUI(const URL& originatingURL
 
         if (showPaymentUIRequestSeed != paymentCoordinatorProxy->m_showPaymentUIRequestSeed) {
             // We've already been asked to hide the payment UI. Don't attempt to show it.
-            return;
+            return completionHandler(false);
         }
 
         ASSERT(viewController);