Attachment 362068 Details for Bug 194678 – Patch

[patch] Patch

bug-194678-20190214151513.patch (text/plain), 13.69 KB, created by Alex Christensen on 2019-02-14 15:15:13 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-02-14 15:15:13 PST

Size: 13.69 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 241566)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,29 @@
>+2019-02-14  Alex Christensen  <achristensen@webkit.org>
>+
>+        Make WebPaymentCoordinatorProxy more robust and modern
>+        https://bugs.webkit.org/show_bug.cgi?id=194678
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-14  Youenn Fablet  <youenn@apple.com>
> 
>         ASSERTION FAILED: m_caches.isEmpty() || !m_pendingInitializationCallbacks.isEmpty() in WebKit::CacheStorage::Caches::clearMemoryRepresentation()
>Index: Source/WebKit/Scripts/webkit/messages.py
>===================================================================
>--- Source/WebKit/Scripts/webkit/messages.py	(revision 241441)
>+++ Source/WebKit/Scripts/webkit/messages.py	(working copy)
>@@ -189,10 +189,8 @@ def forward_declarations_and_headers(rec
>         '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: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.cpp
>===================================================================
>--- Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.cpp	(revision 241441)
>+++ Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.cpp	(working copy)
>@@ -36,7 +36,11 @@
> 
> 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::WebPaymentCo
> 
> 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::openPay
>     });
> }
> 
>-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::showPay
>     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::didReac
>     m_state = State::Idle;
>     m_merchantValidationState = MerchantValidationState::Idle;
> 
>-    ASSERT(activePaymentCoordinatorProxy == this);
>-    activePaymentCoordinatorProxy = nullptr;
>+    ASSERT(activePaymentCoordinatorProxy() == this);
>+    activePaymentCoordinatorProxy() = nullptr;
> }
> 
> }
>Index: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h
>===================================================================
>--- Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h	(revision 241441)
>+++ Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h	(working copy)
>@@ -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: Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.messages.in
>===================================================================
>--- Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.messages.in	(revision 241441)
>+++ Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.messages.in	(working copy)
>@@ -26,12 +26,12 @@
> 
> 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: Source/WebKit/UIProcess/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm
>===================================================================
>--- Source/WebKit/UIProcess/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm	(revision 241441)
>+++ Source/WebKit/UIProcess/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm	(working copy)
>@@ -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: Source/WebKit/UIProcess/ApplePay/mac/WebPaymentCoordinatorProxyMac.mm
>===================================================================
>--- Source/WebKit/UIProcess/ApplePay/mac/WebPaymentCoordinatorProxyMac.mm	(revision 241441)
>+++ Source/WebKit/UIProcess/ApplePay/mac/WebPaymentCoordinatorProxyMac.mm	(working copy)
>@@ -35,16 +35,16 @@
> 
> 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::platfor
> 
>         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);

Flags:

aestes: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194678: 362068