Bug 191298

Created attachment 353947 [details] Patch

Hey guys, please ignore the obvious style errors, and changelog entries for now as those are a work in progress. I just wanted to get this patch out as soon I could get it to build on at least the iOS-simulator, so you guys could start taking a look at what was done. Thanks!

Also, I'll be rebasing it ASAP, so please ignore that as well!

Created attachment 353949 [details] Patch

Fixed most of the obvious style errors.

Created attachment 354162 [details] Patch

Comment on attachment 354162 [details] Patch It'll take me a while to get through this, but one high-level comment: I think this should be called the Payments process (com.apple.WebKit.Payments).

Comment on attachment 354162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354162&action=review > Source/WebKit/PaymentProcess/PaymentProcess.cpp:176 > +void PaymentProcess::fetchWebsiteData(PAL::SessionID sessionID, OptionSet<WebsiteDataType> websiteDataTypes, uint64_t callbackID) Why does the PaymentProcess have these (fetchWebsiteData/deleteWebsiteData/etc.)? > Source/WebKit/UIProcess/API/C/WKContext.h:53 > +typedef WKContextChildProcessDidCrashCallback WKContextDatabaseProcessDidCrashCallback; Database process??

Comment on attachment 354162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354162&action=review > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:77 > +ENABLE_PAYMENT_PROCESS = ; The ENABLE_ flags should be kept in sorted order. > Source/WebKit/PaymentProcess/PaymentProcess.messages.in:36 > + # Initializes a WebsiteDataStore/Session in the PaymentProcess with the correct parameters > + InitializeWebsiteDataStore(struct WebKit::PaymentProcessCreationParameters processCreationParameters) > + > + # Creates a connection for communication with a WebProcess > + CreatePaymentToWebProcessConnection(bool isServiceWorkerProcess, struct WebCore::SecurityOriginData origin) > + > + FetchWebsiteData(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes, uint64_t callbackID) > + DeleteWebsiteData(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes, WallTime modifiedSince, uint64_t callbackID) > + DeleteWebsiteDataForOrigins(PAL::SessionID sessionID, OptionSet<WebKit::WebsiteDataType> websiteDataTypes, Vector<WebCore::SecurityOriginData> origins, uint64_t callbackID) > + > + DestroySession(PAL::SessionID sessionID) These are all storage process messages. Let's start out with an empty message list then add things as we go. > Source/WebKit/Sources.txt:227 > +PaymentProcess/PaymentProcess.cpp @no-unify > +PaymentProcess/PaymentToWebProcessConnection.cpp @no-unify Would be nice to know why these can't be unified. > Source/WebKit/SourcesCocoa.txt:198 > +PaymentProcess/ios/PaymentProcessIOS.mm @no-unify Ditto. > Source/WebKit/UIProcess/Payment/PaymentProcessProxy.cpp:2 > + * Copyright (C) 2013-2018 Apple Inc. All rights reserved. More storage process code in this file that can be removed. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:962 > +TEST(ServiceWorkers, PaymentProcessConnectionCreation) It's awesome to have a test already, but it doesn't belong in this file. > Tools/WebKitTestRunner/TestController.cpp:1066 > +const char* TestController::databaseProcessName() This shouldn't be called databaseProcessName.

(In reply to Andy Estes from comment #7) > Comment on attachment 354162 [details] > Patch > > It'll take me a while to get through this, but one high-level comment: I > think this should be called the Payments process (com.apple.WebKit.Payments). Slight modification to this plan. Let's call the process name com.apple.WebKit.Payments, but when naming C++ classes and whatnot let's use PaymentProcess (and PaymentProcessProxy, etc.).

Created attachment 354191 [details] Patch

Created attachment 354197 [details] Patch

Created attachment 354198 [details] Patch

Created attachment 354226 [details] Patch

Attachment 354226 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 354230 [details] Patch

Attachment 354230 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 67 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 354283 [details] Patch

Created attachment 354407 [details] Patch

Comment on attachment 354407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354407&action=review > Source/WebKit/ChangeLog:106 > + * WebProcess/Storage/WebSWClientConnection.h: > + * WebProcess/Storage/WebToPaymentProcessConnection.cpp: Added. These don't belong in WebProcess/Storage. > Source/WebKit/ChangeLog:113 > + * WebProcess/Storage/WebToPaymentProcessConnection.h: Added. Ditto. > Source/WebCore/en.lproj/Localizable.strings:47 > +/* visible name of the payment process. The argument is the application name. */ > +"%@ Payment" = "%@ Payment"; s/Payment/Payments > Source/WebKit/CMakeLists.txt:36 > + "${WEBKIT_DIR}/Shared/Payment" > "${WEBKIT_DIR}/Shared/WebsiteData" > + "${WEBKIT_DIR}/PaymentProcess" These should be in sorted order. > Source/WebKit/CMakeLists.txt:52 > + "${WEBKIT_DIR}/UIProcess/Payment" Ditto. > Source/WebKit/Configurations/Payment-OSX-sandbox.entitlements:5 > + <key>com.apple.rootless.payment.WebKitPaymentSandbox</key> Not sure what this file does. Maybe we should talk to rniwa about this? > Source/WebKit/DerivedSources.make:40 > + $(WebKit2)/PaymentProcess \ Needs to be sorted. > Source/WebKit/DerivedSources.make:138 > + PaymentProcess \ > + PaymentProcessProxy \ > + PaymentToWebProcessConnection \ Ditto. > Source/WebKit/PaymentProcess/PaymentProcess.cpp:46 > +#include "ChildProcessMessages.h" > +#include "Logging.h" > +#include "PaymentProcessCreationParameters.h" > +#include "PaymentProcessMessages.h" > +#include "PaymentProcessProxyMessages.h" > +#include "PaymentToWebProcessConnection.h" > +#include "WebCoreArgumentCoders.h" > +#include "WebsiteData.h" > +#include <WebCore/FileSystem.h> > +#include <WebCore/NotImplemented.h> > +#include <WebCore/TextEncoding.h> > +#include <pal/SessionID.h> > +#include <wtf/CallbackAggregator.h> > +#include <wtf/CrossThreadTask.h> > +#include <wtf/MainThread.h> > +#include <wtf/MemoryPressureHandler.h> After doing the other cleanups, I'd remove these includes by one and only keep the ones that are necessary to build. > Source/WebKit/PaymentProcess/PaymentProcess.cpp:59 > + : m_queue(WorkQueue::create("com.apple.WebKit.Payments")) I don't think we need a WorkQueue yet. > Source/WebKit/PaymentProcess/PaymentProcess.cpp:63 > + // Make sure the UTF8Encoding encoding and the text encoding maps have been built on the main thread before a background thread needs it. > + // FIXME: https://bugs.webkit.org/show_bug.cgi?id=135365 - Need a more explicit way of doing this besides accessing the UTF8Encoding. > + UTF8Encoding(); Not sure we need this either so long as we don't have background threads. > Source/WebKit/PaymentProcess/PaymentProcess.cpp:129 > +void PaymentProcess::ensurePathExists(const String& path) > +{ > + ASSERT(!RunLoop::isMain()); > + > + if (!FileSystem::makeAllDirectories(path)) > + LOG_ERROR("Failed to make all directories for path '%s'", path.utf8().data()); > +} > + > +void PaymentProcess::postPaymentTask(CrossThreadTask&& task) > +{ > + ASSERT(RunLoop::isMain()); > + > + LockHolder locker(m_paymentTaskMutex); > + > + m_paymentTasks.append(WTFMove(task)); > + > + m_queue->dispatch([this] { > + performNextPaymentTask(); > + }); > +} > + > +void PaymentProcess::performNextPaymentTask() > +{ > + ASSERT(!RunLoop::isMain()); > + > + CrossThreadTask task; > + { > + LockHolder locker(m_paymentTaskMutex); > + ASSERT(!m_paymentTasks.isEmpty()); > + task = m_paymentTasks.takeFirst(); > + } > + > + task.performTask(); > +} Don't think we need any of this. > Source/WebKit/PaymentProcess/PaymentProcess.cpp:137 > +#if USE(UNIX_DOMAIN_SOCKETS) > + IPC::Connection::SocketPair socketPair = IPC::Connection::createPlatformConnection(); > + m_paymentToWebProcessConnections.append(PaymentToWebProcessConnection::create(socketPair.server)); > + parentProcessConnection()->send(Messages::PaymentProcessProxy::DidCreatePaymentToWebProcessConnection(IPC::Attachment(socketPair.client)), 0); > +#elif OS(DARWIN) This feature is Cocoa-only for now, so we don't need the USE(UNIX_DOMAIN_SOCKETS) part of this. > Source/WebKit/PaymentProcess/PaymentProcess.cpp:191 > +#if !PLATFORM(COCOA) > +void PaymentProcess::initializeProcess(const ChildProcessInitializationParameters&) > + { > +#if OS(LINUX) > + auto& memoryPressureHandler = MemoryPressureHandler::singleton(); > + memoryPressureHandler.setLowMemoryHandler([this] (Critical, Synchronous) { > + // FIXME: no lowMemoryHandler() implemented for PaymentProcess currently. > + // But at least define this setLowMemoryHandler() empty so platformReleaseMemory is called. > + }); > + memoryPressureHandler.install(); > +#endif > + } > +void PaymentProcess::destroySession(PAL::SessionID sessionID) > +{ > +} > + > +void PaymentProcess::initializeProcessName(const ChildProcessInitializationParameters&) > +{ > +} > + > +void PaymentProcess::initializeSandbox(const ChildProcessInitializationParameters&, SandboxInitializationParameters&) > +{ > +} > +#endif // !PLATFORM(COCOA) Surely don't need any of this right now, because PaymentProcess will start out Cocoa-only. > Source/WebKit/PaymentProcess/PaymentProcess.h:25 > + */ > +#if ENABLE(PAYMENT_PROCESS) Missing a blank line before the #if. The pragma should also be above the #if. > Source/WebKit/PaymentProcess/PaymentProcess.h:49 > + void postPaymentTask(CrossThreadTask&&); Don't think you'll need this. > Source/WebKit/PaymentProcess/PaymentProcess.h:67 > + void initializeWebsiteDataStore(const PaymentProcessCreationParameters&); Ditto. > Source/WebKit/PaymentProcess/PaymentProcess.h:81 > + void destroySession(PAL::SessionID); > + > + // For execution on work queue thread only > + void performNextPaymentTask(); > + void ensurePathExists(const String&); > + > + Vector<Ref<PaymentToWebProcessConnection>> m_paymentToWebProcessConnections; > + > + Ref<WorkQueue> m_queue; > + > + Deque<CrossThreadTask> m_paymentTasks; > + Lock m_paymentTaskMutex; Ditto. > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.cpp:53 > + // Use this flag to force synchronous messages to be treated as asynchronous messages in the WebProcess. > + // Otherwise, the WebProcess would process incoming synchronous IPC while waiting for a synchronous IPC > + // reply from the payment process, which would be unsafe. > + m_connection->setOnlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage(true); > + m_connection->open(); Maybe we need this, but I don't know right now. I'd replace this with a FIXME saying to investigate whether we need to call setOnlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage(true). > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.cpp:83 > + Surprised this didn't need UNUSED_PARAMSs. > Source/WebKit/PaymentProcess/ios/PaymentProcessIOS.mm:29 > +#if PLATFORM(IOS_FAMILY) I think this should just be PLATFORM(IOS) for now. > Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.Payments.sb:47 > +(version 1) > +(deny default (with partial-symbolication)) > +(allow system-audit file-read-metadata) > + > +(import "common.sb") > + > +(deny mach-lookup (xpc-service-name-prefix "")) > + > +(deny lsopen) > + > +(allow file-read* file-write* (extension "com.apple.app-sandbox.read-write")) > + > +(deny sysctl*) > +(allow sysctl-read > + (sysctl-name > + "hw.availcpu" > + "hw.ncpu" > + "hw.model" > + "kern.memorystatus_level" > + "vm.footprint_suspend")) > + > +;; Various services required by system frameworks > +(allow mach-lookup > + (global-name "com.apple.analyticsd")) I don't know if this is right. I think we should start with the most restrictive seatbelt profile possible then expand it from there. Maybe for now this should just do the "(import "common.sb)"? Maybe Brent Fulgham can help us with this part. > Source/WebKit/Sources.txt:223 > +Shared/Payment/PaymentProcessCreationParameters.cpp > + Sorted order please. > Source/WebKit/Sources.txt:227 > +PaymentProcess/PaymentProcess.cpp @no-unify > +PaymentProcess/PaymentToWebProcessConnection.cpp @no-unify Ditto. Also, why @no-unify? > Source/WebKit/Sources.txt:390 > +UIProcess/Payment/PaymentProcessProxy.cpp > + Ditto. > Source/WebKit/SourcesCocoa.txt:199 > + > +PaymentProcess/ios/PaymentProcessIOS.mm @no-unify > + Ditto. > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:65 > + default: return nil; We don't like adding defaults since they silence compiler warnings about missing enumeration values. Also, shouldn't you actually handle ProcessType::Payment here? > Source/WebKit/UIProcess/Payment/PaymentProcessProxy.cpp:138 > +#if USE(UNIX_DOMAIN_SOCKETS) > + reply(connectionIdentifier); > +#elif OS(DARWIN) > + reply(IPC::Attachment(connectionIdentifier.port(), MACH_MSG_TYPE_MOVE_SEND)); > +#elif OS(WINDOWS) > + reply(connectionIdentifier.handle()); > +#else No need for Unix or Windows code here. > Source/WebKit/WebProcess/Storage/WebSWClientConnection.h:93 > + void schedulePaymentJob(const WebCore::ServiceWorkerJobData&); This doesn't belong here. > Source/WebKit/WebProcess/WebProcess.cpp:1247 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + // GTK+ and WPE ports don't exit on send sync message failure. > + // In this particular case, the payment process can be terminated by the UI process while the > + // connection is being done, so we always want to exit instead of crashing. > + // See https://bugs.webkit.org/show_bug.cgi?id=183348. > + exit(0); > +#else No need for this. > Source/WebKit/WebProcess/WebProcess.cpp:1258 > +#if USE(UNIX_DOMAIN_SOCKETS) > + IPC::Connection::Identifier connectionIdentifier = encodedConnectionIdentifier.releaseFileDescriptor(); > +#elif OS(DARWIN) > + IPC::Connection::Identifier connectionIdentifier(encodedConnectionIdentifier.port()); > +#elif OS(WINDOWS) > + IPC::Connection::Identifier connectionIdentifier(encodedConnectionIdentifier.handle()); > +#else No need for Unix and Windows code here. > Tools/WebKitTestRunner/TestController.cpp:1069 > +#if PLATFORM(IOS_FAMILY) PLATFORM(IOS) > Tools/WebKitTestRunner/TestController.cpp:1070 > + return "com.apple.WebKit.Payment"; com.apple.WebKit.Payments. > LayoutTests/http/wpt/service-workers/persistent-importScripts.html:42 > - if (window.testRunner) > + if (window.testRunner) { > testRunner.terminateNetworkProcess(); > + testRunner.terminatePaymentProcess(); > + } Don't think you need to do this.

Comment on attachment 354407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354407&action=review > Source/JavaScriptCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=191298 Please always provide a radar for your work. >> Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.Payments.sb:47 >> + (global-name "com.apple.analyticsd")) > > I don't know if this is right. I think we should start with the most restrictive seatbelt profile possible then expand it from there. Maybe for now this should just do the "(import "common.sb)"? Maybe Brent Fulgham can help us with this part. Yes. I'd get rid of everything below "(deny lsopen)" and then see what sandbox error messages you get as you run your tests. We can open the sandbox to support those needed system calls, rather than start with some boilerplate version that might include things you don't need.

Created attachment 354858 [details] Patch

Created attachment 354883 [details] Patch

Created attachment 354885 [details] Patch

Created attachment 354900 [details] Patch

Created attachment 354966 [details] Patch

Created attachment 354983 [details] Patch

Comment on attachment 354983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354983&action=review Cool! > Source/WebKit/Configurations/PaymentService.xcconfig:27 > +WK_PAYMENT_ENTITLEMENTS_RESTRICTED_YES = Configurations/Payment-OSX-sandbox.entitlements; This should probably be removed. It refers to a file that doesn't exist. When we need to give the process entitlements, we can add such a thing. > Source/WebKit/PaymentProcess/PaymentProcess.messages.in:25 > +messages -> PaymentProcess LegacyReceiver { Please don't add any new LegacyReceivers. > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.cpp:52 > + // to force synchronous messages to be treated as asynchronous messages in the WebProcess. This isn't exactly true. It forces synchronous messages to not cause deadlock, but they're still synchronous. > Source/WebKit/UIProcess/API/C/WKContextPrivate.h:95 > +// NOTE: This is only available when PAYMENT_PROCESS is ENABLED This should be removed. Just make the SPI not do anything when the feature is disabled. > Source/WebKit/UIProcess/Payments/PaymentProcessProxy.cpp:42 > +static uint64_t generatePaymentProcessCallbackID() This should not be necessary. Use sendWithAsyncReply instead. In this patch it can just be removed because it isn't used. > Source/WebKit/UIProcess/WebProcessPool.cpp:630 > +void WebProcessPool::paymentProcessCrashed(PaymentProcessProxy* paymentProcessProxy) This should be a PaymentProcessProxy& > Source/WebKit/UIProcess/WebProcessPool.cpp:724 > + sendToPaymentProcess(Messages::PaymentProcess::DestroySession(PAL::SessionID::legacyPrivateSessionID())); Can we get away with not supporting the legacy private session id in the payment process, or is it really needed for testing? Let's not add it if we can. We're trying to remove uses of legacyProvateSessionID. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:143 > + processPool->sendToPaymentProcess(Messages::PaymentProcess::DestroySession(m_sessionID)); This should probably be wrapped in a function like the network process has. Having the same abstraction seems like a good idea in this case. > Tools/WebKitTestRunner/TestController.cpp:1082 > + return "Payment process not available on anything but iOS"; Why not? It seems like we would want to implement this on Mac, too.

Comment on attachment 354983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354983&action=review > Source/WebKit/PaymentProcess/PaymentProcess.cpp:82 > +#endif // !PAYMENT(PAYMENT_PROCESS) // ENABLE(PAYMENT_PROCESS) > Source/WebKit/PaymentProcess/PaymentProcess.h:69 > +#endif // !PAYMENT(PAYMENT_PROCESS) // ENABLE(PAYMENT_PROCESS) > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.cpp:92 > +#endif // !PAYMENT(PAYMENT_PROCESS) // ENABLE(PAYMENT_PROCESS) > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.h:64 > +#endif // !PAYMENT(PAYMENT_PROCESS) // ENABLE(PAYMENT_PROCESS) > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:88 > +- (void)_terminatePaymentProcess; This one needs availability macros (look for WK_IOS_TBA and WK_MAC_TBA) > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:96 > +- (pid_t)_paymentProcessIdentifier WK_API_AVAILABLE(ios(11.3)); This should use TBA availability macros. > Source/WebKit/WebProcess/Storage/WebSWClientConnection.h:93 > + void schedulePaymentJob(const WebCore::ServiceWorkerJobData&); Are we sure we need this? Looks like this is not implemented anywhere. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1050 > + // Make sure payment process is not launched. > + EXPECT_EQ(0, webView.get().configuration.processPool._paymentProcessIdentifier); It's unclear to me how the payment process would be relevant to this test.

(In reply to Alex Christensen from comment #28) > Comment on attachment 354983 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=354983&action=review > > > Tools/WebKitTestRunner/TestController.cpp:1082 > > + return "Payment process not available on anything but iOS"; > > Why not? It seems like we would want to implement this on Mac, too. Maybe we will implement this on Mac one day, but right now we don't have the PassKit support do this anywhere other than on iOS.

Created attachment 355158 [details] Patch

Created attachment 355593 [details] Patch

Created attachment 355595 [details] Patch

Created attachment 355601 [details] Patch

What is the goal of this new process? Why is it needed?

Comment on attachment 355601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355601&action=review > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.cpp:52 > + // FIXME: > + // Use m_connection->setOnlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage(true) > + // to prevent deadlock while using synchronous messages I hope we won't need this. > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.messages.in:24 > +messages -> PaymentToWebProcessConnection LegacyReceiver { Please remove LegacyReceiver. > Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:459 > + return (pid_t)0; You shouldn't need the cast. > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:87 > +// NOTE: This is only available when PAYMENT_PROCESS is ENABLED This comment isn't necessary. > Source/WebKit/UIProcess/Payments/PaymentProcessProxy.cpp:103 > + m_processPool.paymentProcessCrashed(this); Pass *this so the callee knows it can't be null. Also, the header and implementation don't line up right now. > Source/WebKit/UIProcess/Payments/PaymentProcessProxy.messages.in:24 > +messages -> PaymentProcessProxy LegacyReceiver { Please no LegacyReceiver.

Comment on attachment 355601 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355601&action=review >> Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.cpp:52 >> + // to prevent deadlock while using synchronous messages > > I hope we won't need this. Actually, this NEEDS to be set to true right now. This is not about preventing deadlocks. Unless you set this to true, the WebContent process may re-enter when sending a sync IPC, which is a huge source of security bugs.

Created attachment 355675 [details] Patch

Created attachment 355679 [details] Patch

(In reply to Sam Weinig from comment #35) > What is the goal of this new process? Why is it needed? We were planning on experimenting with PKPaymentAuthorizationController on iOS.

Comment on attachment 355679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355679&action=review > Source/WebKit/Configurations/PaymentService.xcconfig:33 > +INFOPLIST_FILE = ; We will need an Info.plist file for this service as well as an entry point file. For instance, look at how INFOPLIST_FILE is set in NetworkService.xcconfig, then look at Source/WebKit/NetworkProcess/EntryPoint/. > Source/WebKit/DerivedSources.make:126 > + NetworkContentRuleListManager \ NetworkContentRuleListManager? > Source/WebKit/PaymentProcess/PaymentProcess.cpp:29 > +#include "config.h" > +#include "PaymentProcess.h" These two lines should go above the ENABLE(PAYMENT_PROCESS) line. > Source/WebKit/PaymentProcess/PaymentToWebProcessConnection.cpp:29 > +#include "config.h" > +#include "PaymentToWebProcessConnection.h" Ditto. > Source/WebKit/PaymentProcess/ios/PaymentProcessIOS.mm:27 > +#import "config.h" Ditto. > Source/WebKit/UIProcess/WebProcessPool.h:180 > + // Sends the message to WebProcess or PaymentProcess as approporiate for current process model. This comment isn't right. These will always send to the PaymentProcess. > Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetController.cpp:66 > - InspectorTarget* target = m_targets.get(targetId); > + auto* target = m_targets.get(targetId); This change doesn't seem related. > Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetController.cpp:81 > - InspectorTarget* target = m_targets.get(targetId); > + auto* target = m_targets.get(targetId); Ditto. > Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetController.cpp:94 > - InspectorTarget* target = m_targets.get(targetId); > + auto* target = m_targets.get(targetId); Ditto. > Source/WebKit/WebProcess/WebProcess.cpp:48 > +#if ENABLE(PAYMENT_PROCESS) > +#include "PaymentProcessMessages.h" > +#endif // ENABLE(PAYMENT_PROCESS) Please group the ENABLE(PAYMENT_PROCESS) includes together rather than scattering them into the unconditional includes (see below for other examples). > Source/WebKit/WebProcess/WebProcess.cpp:77 > +#if ENABLE(PAYMENT_PROCESS) > +#include "WebToPaymentProcessConnection.h" > +#endif // ENABLE(PAYMENT_PROCESS) Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:147 > + EXPECT_EQ((pid_t)0, [sharedProcessPool _paymentProcessIdentifier]); I don't think we need this. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:172 > + EXPECT_EQ((pid_t)0, [sharedProcessPool _paymentProcessIdentifier]); Ditto. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm:1135 > + // There should be no payment process created. > + EXPECT_EQ(0, webView.get().configuration.processPool._paymentProcessIdentifier); > + Ditto. > Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:338 > + void terminatePaymentProcess(); This should be annotated with [Conditional=PAYMENT_PROCESS]. > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:1300 > +void TestRunner::terminatePaymentProcess() > +{ > + WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("TerminatePaymentProcess")); > + WKBundlePagePostSynchronousMessageForTesting(InjectedBundle::singleton().page()->page(), messageName.get(), nullptr, nullptr); > +} Wrap this in #if ENABLE(PAYMENT_PROCESS). > Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:451 > + void terminatePaymentProcess(); Ditto. > Tools/WebKitTestRunner/TestController.cpp:1083 > +const char* TestController::paymentProcessName() > +{ > +#if PLATFORM(FAMILY_IOS) > + return "com.apple.WebKit.Payments"; > +#else > + return "Payment process not available on anything but iOS"; > +#endif > +} This is not right. Have the function always return "com.apple.WebKit.Payments", then wrap the function in #if ENABLE(PAYMENT_PROCESS). > Tools/WebKitTestRunner/TestController.cpp:1522 > +void TestController::paymentProcessDidCrash(WKContextRef context, const void *clientInfo) > +{ > + static_cast<TestController*>(const_cast<void*>(clientInfo))->paymentProcessDidCrash(); > +} I think this function is supposed to be registered as a WKContextClient callback, but it doesn't look like you wired that up. > Tools/WebKitTestRunner/TestController.cpp:2667 > +void TestController::terminatePaymentProcess() > +{ > + WKContextTerminatePaymentProcess(platformContext()); > +} > + Wrap this in #if ENABLE(PAYMENT_PROCESS). > Tools/WebKitTestRunner/TestController.h:184 > + static const char* paymentProcessName(); Ditto. > Tools/WebKitTestRunner/TestController.h:256 > + void terminatePaymentProcess(); Ditto. > Tools/WebKitTestRunner/TestInvocation.cpp:1461 > + if (WKStringIsEqualToUTF8CString(messageName, "TerminatePaymentProcess")) { > + ASSERT(!messageBody); > + TestController::singleton().terminatePaymentProcess(); > + return nullptr; > + } > + Ditto.

Comment on attachment 355679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355679&action=review >> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetController.cpp:66 >> + auto* target = m_targets.get(targetId); > > This change doesn't seem related. I think these miiiight be needed as unified-source-related build fixes. >> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetController.cpp:81 >> + auto* target = m_targets.get(targetId); > > Ditto. (This too) >> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetController.cpp:94 >> + auto* target = m_targets.get(targetId); > > Ditto. (and this)

Created attachment 355837 [details] Patch

Comment on attachment 355837 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355837&action=review > Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig:293 > +ENABLE_PAYMENT_PROCESS = ; This does precisely nothing. If you want it to do anything at all, it has to be down below in the long FEATURE_DEFINES line too. ALSO, why is it disabled? It’s weird to add so much code and not even build it anywhere. > Source/WebCore/PAL/Configurations/FeatureDefines.xcconfig:294 > ENABLE_PAYMENT_REQUEST = ENABLE_PAYMENT_REQUEST; Then again, maybe that’s OK. > Source/WebKit/Configurations/BaseTarget.xcconfig:114 > +WK_PAYMENT_SERVICE_PRODUCT_NAME = com.apple.WebKit.Payments; I wonder why this is here? I see the storage one below; are the *other* process names in BaseTarget or in their own xcconfigs? > Source/WebKit/Configurations/PaymentService.xcconfig:33 > +INFOPLIST_FILE[sdk=iphone*] = PaymentProcess/EntryPoint/ios/XPCService/PaymentService/Info-iOS.plist; SDK conditionals like this are a big red flag w.r.t. PLATFORM(IOSMAC), you probably want to use WK_PLATFORM_NAME. Are the others like this? > Source/WebKit/PaymentProcess/PaymentProcess.h:39 > +struct PaymentProcessCreationParameters; You have this, plus a whole new file for this struct, and yet as far as I can tell, nothing uses it?? > Source/WebKit/UIProcess/API/C/WKContext.h:66 > + WKContextPaymentProcessDidCrashCallback paymentProcessDidCrash; You cannot change these structs; old versions must be immutable for source compatibility. You need to add a new version (which is quite a bit of typing. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1644 > +PaymentProcessCreationParameters WebsiteDataStore::paymentProcessParameters() Ditto my previous question about PaymentProcessCreationParameters... nothing ever calls this function? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:69 > +#if ENABLE(PAYMENT_PROCESS) We usually like to put things like this in their own paragraph (and they then break out of alphabetical order too. See e.g. RESOURCE_LOAD_STATISTICS and NETSCAPE_PLUGIN_API below. > Source/WebKit/WebKit.xcodeproj/project.pbxproj:6305 > + 3913602A219514BD0056492A /* Payment */ = { Payment... or ... > Source/WebKit/WebKit.xcodeproj/project.pbxproj:6314 > + 392E942A219A2E850050F982 /* Payments */ = { ...Payments? > Source/WebKit/WebProcess/Payments/WebToPaymentProcessConnection.h:33 > +#include <WebCore/SWServer.h> Why ServiceWorkers headers?

Created attachment 357759 [details] Patch

Created attachment 357829 [details] Patch 2

Attachment 357829 [details] did not pass style-queue: ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.h:56: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxy.h:136: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxy.h:151: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:40: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:69: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:117: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:121: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:136: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:149: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:163: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:177: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:266: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:358: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:366: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:402: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:412: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:581: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:608: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:617: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:660: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] ERROR: Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxyCocoa.mm:677: Misplaced OS version check. Please use a named macro in wtf/Platform.h or wtf/FeatureDefines.h. [build/version_check] [5] Total errors found: 21 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 357834 [details] Patch #2

This patch should have a much smaller diff.

Comment on attachment 357834 [details] Patch #2 View in context: https://bugs.webkit.org/attachment.cgi?id=357834&action=review > Source/WebCore/ChangeLog:10 > +2018-11-14 Zamiul Haque <zhaque@apple.com> Double changelogs > Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxy.cpp:148 > + // FIXME: This should be a MESSAGE_CHECK. Why isn't it, then? > Source/WebKit/PaymentProcess/PaymentProcessCoordinatorProxy.cpp:183 > + // It's possible that the payment has been canceled already. Don't know that we need this comment every time. Also is it even accurate in this case? The other ones about cancellation check m_state, this one is checking canCompletePayment() > Source/WebKit/WebKit.xcodeproj/project.pbxproj:11689 > + "INFOPLIST_FILE[sdk=macosx*]" = "PaymentProcess/EntryPoint/ios/XPCService/PaymentService/Info-iOS.plist"; Info-iOS for macosx seems wrong. Also should these even be in the xcproj? Aren't they already in the xcconfigs?

Created attachment 357876 [details] Patch #2

Created attachment 358014 [details] Patch #2

Still relevant?