WebKit Bugzilla
Attachment 362068 Details for
Bug 194678
: Make WebPaymentCoordinatorProxy more robust and modern
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch
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);
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Flags:
aestes
:
review+
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 194678
: 362068