Reuse existing web processes for running service workers
Created attachment 379534 [details] Patch
Created attachment 379540 [details] Fix API tests
Created attachment 379630 [details] Patch
Comment on attachment 379630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379630&action=review > Source/WebKit/ChangeLog:26 > + We no longer terminate the web process when stopping service workers or when network process crash. Is a runaway ServiceWorker still terminated when it has no clients?
(In reply to Geoffrey Garen from comment #4) > Comment on attachment 379630 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=379630&action=review > > > Source/WebKit/ChangeLog:26 > > + We no longer terminate the web process when stopping service workers or when network process crash. > > Is a runaway ServiceWorker still terminated when it has no clients? Yes, we reuse the existing way to terminate service workers which if stop is not done after a timeout will make the process exit. This is ok in that particular case since there is no client/page in the given process. In other cases, like when marking a worker as redundant, we might actually exit the process that is running some pages. This seems ok to me since we are partitioning by registrable domain and a bad service worker might anyway make the pages disfunction as well.
Ping review?
Comment on attachment 379630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379630&action=review > Source/WebKit/UIProcess/WebProcessProxy.cpp:1445 > + disableServiceWorkers(); I don't understand why this is here. > LayoutTests/ChangeLog:3 > + ServiceWorkerFetchTask can use the NetworkConnectionToWebProcess sessionID Inconsistent ChangeLog title.
Comment on attachment 379630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379630&action=review >> Source/WebKit/UIProcess/WebProcessProxy.cpp:1445 >> + disableServiceWorkers(); > > I don't understand why this is here. This is not strictly necessary. AIUI, this code means that we are about to run JavaScript from various domains in this process. In that case, it seems better to stop all service workers running in this process. They will be relaunched in a process with a matching registrable domain.
Comment on attachment 379630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379630&action=review This mostly seems good. If we are writing code that says "an existing web process can be reused for a service worker later", then I think we should have at least one API test that verifies that precise behavior. > Source/WebCore/workers/service/context/SWContextManager.h:98 > + static constexpr Seconds workerTerminationTimeoutForTest { 100_ms }; // Only used by layout tests. I don't think workerTerminationTimeoutForTest is a better name than syncWorkerTerminationTimeout Sync termination is not only about testing.
Created attachment 380209 [details] Patch
Created attachment 380216 [details] Patch
Created attachment 380221 [details] Patch
Ping review
Comment on attachment 380221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380221&action=review > Source/WebCore/testing/Internals.cpp:5237 > +int Internals::processId() const I think this should either be processID or processIdentifier, I have a preference for the latter. > Source/WebCore/workers/service/context/SWContextManager.h:72 > + bool m_isStopped { false }; Protected data members are not great practiced. I think this should be private. If it needs to be set from a subclass, then introduce a protected setter. > Source/WebCore/workers/service/context/SWContextManager.h:98 > + static constexpr Seconds syncWorkerTerminationTimeout { 100_ms }; // Only used by layout tests. Doesn't Brady's patch from a couple days ago use sync worker termination is non-testing cases, when a service worker that not handle a fetch in time? > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:895 > + RELEASE_LOG(ServiceWorker, "WebProcess %llu no longer useful for running service workers", webProcessIdentifier().toUInt64()); We should not be logging in ephemeral sessions. NetworkConnectionToWebProcess has a sessionID for you to do the check. > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:114 > +- (void)_setForceSeparateServiceWorkerProcess:(BOOL)forceServiceWorkerProcess WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); Why do we need this? Having to support both separate and merged models increases code complexity. > Source/WebKit/UIProcess/WebProcessPool.cpp:729 > + RELEASE_LOG(ServiceWorker, "WebProcessPool::establishWorkerContextConnectionToNetworkProcess reusing an existing web process %p", serviceWorkerProcessProxy); Please log PID too. > Source/WebKit/UIProcess/WebProcessPool.cpp:738 > + RELEASE_LOG(ServiceWorker, "WebProcessPool::establishWorkerContextConnectionToNetworkProcess creating a new service worker process %p", serviceWorkerProcessProxy); Please log PID too. > Source/WebKit/UIProcess/WebProcessPool.cpp:755 > + RegistrableDomainWithSessionID key { process.registrableDomain(), process.websiteDataStore().sessionID() }; The process' registrable domain can change throughout the lifetime of a WebProcessProxy, this is therefore unsafe. In particular, if you do a load for a given domainA in a WebContent process, then a WebContent process's registrable domain becomes domainA. If later on, you do a load for domainB in this same process, then the web process's registrable domain gets nulled out (since the WebContent process is no longer associated with a single registrable domain). There are many reasons right now causing us to load different domains in a single process, since we are sometimes unable to process swap on navigation (e.g. due to opener links). > Source/WebKit/UIProcess/WebProcessPool.cpp:1141 > + auto iterator = m_serviceWorkerProcesses.find(RegistrableDomainWithSessionID { process->registrableDomain(), process->websiteDataStore().sessionID() }); Using process->registrableDomain() as key is unsafe here as mentioned earlier. > Source/WebKit/UIProcess/WebProcessPool.cpp:2538 > +void WebProcessPool::setForceSeparateServiceWorkerProcess(bool forceServiceWorkerProcess) Missing "Separate" in the parameter name. > Source/WebKit/UIProcess/WebProcessPool.h:530 > + void setForceSeparateServiceWorkerProcess(bool forceServiceWorkerProcess); Parameter name should be omitted. > Source/WebKit/UIProcess/WebProcessPool.h:810 > + bool m_forceServiceWorkerProcess { false }; Bad naming: - Missing a prefix for boolean variables - You're missing "Separate" in there > Source/WebKit/UIProcess/WebProcessProxy.cpp:923 > + if (isRunningServiceWorkers()) 2 issues here: - Why isn't this in canTerminateAuxiliaryProcess() with the other conditions? - If you add a new condition for not shutting down a process, then it usually means you need to add a call to maybeShutdown() when this condition may have changed. I do not see any new calls to maybeShutdown() in this patch, which raises a red flag. You should likely be calling maybeShutdown() when a process stops running service workers (unless we are already and I merely do not see it in this patch). > Source/WebKit/UIProcess/WebProcessProxy.cpp:1445 > + disableServiceWorkers(); Oh, I see now that you deal with the registrable domain's getting reset here. Looks like this makes sure we remove the process from the HashMap before its registrable domain. Please disregard my previous comments about the registrable domain changing then. This looks fragile but acceptable. > Source/WebKit/UIProcess/WebProcessProxy.cpp:1548 > + if (m_processPool) m_processPool should never be null. Just use the processPool() getter which returns a ref. > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:99 > + WebProcess::singleton().disableTermination(); Please explain in the change log why this is needed. It is not obvious to me why and why it is OK, mostly because I am unfamiliar with disableTermination. > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h:98 > + void stop(); I am unclear what it means to "stop" a *connection*. Terminate or closing a connection makes sense, but stopping is less clear.
> > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:114 > > +- (void)_setForceSeparateServiceWorkerProcess:(BOOL)forceServiceWorkerProcess WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); > > Why do we need this? Having to support both separate and merged models > increases code complexity. The added complexity is limited to an if check in WebProcessPool::establishWorkerContextConnectionToNetworkProcess. We use this for instance in layout tests to check the effect of a service worker crash on a service worker client. We also use this in API tests. > > Source/WebKit/UIProcess/WebProcessProxy.cpp:923 > > + if (isRunningServiceWorkers()) > > 2 issues here: > - Why isn't this in canTerminateAuxiliaryProcess() with the other conditions? > - If you add a new condition for not shutting down a process, then it > usually means you need to add a call to maybeShutdown() when this condition > may have changed. I do not see any new calls to maybeShutdown() in this > patch, which raises a red flag. You should likely be calling maybeShutdown() > when a process stops running service workers (unless we are already and I > merely do not see it in this patch). The principle is to use enableTermination/disableTermination which kicks in whenever network process decides that a given connection is no longer needed. WebProcessProxy will disable service workers, which will make WebProcess close the connection to UIProcess if there is no page running in the web process. Since we have this logic in UIProcess as well, it is indeed more consistent to also do the same in UIProcess. > > Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp:99 > > + WebProcess::singleton().disableTermination(); > > Please explain in the change log why this is needed. It is not obvious to me > why and why it is OK, mostly because I am unfamiliar with disableTermination. disableTermination/enableTermination is used in WebProcess whenever a page is created/removed. We reuse the same mechanism for service workers by making the service worker connection counts as a page.
Created attachment 380637 [details] Patch
If the code path is used only for testing and not a configuration we want to ship, I do not know why we are keeping it. The complexity is not just an if check, it is keeping a decent amount of extra code around and not knowing which code path is being taken.
(In reply to Chris Dumez from comment #17) > If the code path is used only for testing and not a configuration we want to > ship, I do not know why we are keeping it. This is basically a runtime flag that can be switched on and off. This seems actually good to me to have in case we discover issues. I guess we could try to go with WebPreferences. > The complexity is not just an if > check, it is keeping a decent amount of extra code around and not knowing > which code path is being taken. There is an if check and getters/setters. Other than that, I am not sure to see what additional complexity this switch is adding. In terms of complexity, I agree the approach to reuse existing processes adds some complexity. In particular, in some cases, service workers will run in the same process as a page and sometimes they will not. This is independent though of the switch to enable this approach or not.
Comment on attachment 380637 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380637&action=review > Tools/ChangeLog:9 > + This is useful for tests trying to crash the service worker process. Can't those tests simply window.open() another origin to get a separate service worker process?
> Can't those tests simply window.open() another origin to get a separate > service worker process? We could probably do that, until we support full process isolation, where it might break. I honestly do not really see the added complexity of this boolean. I am also thinking of being able to turn the feature on/off for perf comparison as part of the Debug menu.
Comment on attachment 380637 [details] Patch Rejecting attachment 380637 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 380637, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: . Fetching: https://bugs.webkit.org/attachment.cgi?id=380637&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=202195&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 380637 from bug 202195. Fetching: https://bugs.webkit.org/attachment.cgi?id=380637 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Dumez']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 47 diffs from patch file(s). patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/testing/Internals.cpp Hunk #2 succeeded at 5252 with fuzz 2 (offset 4 lines). patching file Source/WebCore/testing/Internals.h Hunk #1 succeeded at 892 (offset 2 lines). patching file Source/WebCore/testing/Internals.idl patching file Source/WebCore/testing/ServiceWorkerInternals.cpp patching file Source/WebCore/testing/ServiceWorkerInternals.h patching file Source/WebCore/testing/ServiceWorkerInternals.idl patching file Source/WebCore/workers/service/ServiceWorkerProvider.cpp patching file Source/WebCore/workers/service/context/SWContextManager.cpp patching file Source/WebCore/workers/service/context/SWContextManager.h patching file Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp patching file Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h patching file Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in patching file Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp Hunk #6 FAILED at 141. 1 out of 6 hunks FAILED -- saving rejects to file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp.rej patching file Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.h patching file Source/WebKit/UIProcess/API/C/WKContext.cpp patching file Source/WebKit/UIProcess/API/C/WKContextPrivate.h patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm Hunk #1 succeeded at 448 (offset 1 line). Hunk #2 succeeded at 509 (offset 1 line). patching file Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.h patching file Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in patching file Source/WebKit/UIProcess/WebProcessPool.cpp Hunk #1 succeeded at 671 (offset 1 line). Hunk #2 succeeded at 702 (offset 1 line). Hunk #3 succeeded at 1134 (offset -1 lines). Hunk #4 succeeded at 1151 (offset -1 lines). Hunk #5 succeeded at 1240 (offset -1 lines). Hunk #6 succeeded at 1719 (offset -3 lines). Hunk #7 succeeded at 2454 (offset -79 lines). patching file Source/WebKit/UIProcess/WebProcessPool.h Hunk #1 succeeded at 387 (offset 3 lines). Hunk #2 succeeded at 520 (offset -8 lines). Hunk #3 succeeded at 797 (offset -10 lines). patching file Source/WebKit/UIProcess/WebProcessProxy.cpp Hunk #2 succeeded at 962 with fuzz 1 (offset 3 lines). Hunk #3 succeeded at 1469 (offset 3 lines). Hunk #4 succeeded at 1568 (offset 5 lines). patching file Source/WebKit/UIProcess/WebProcessProxy.h Hunk #1 succeeded at 123 (offset -2 lines). patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.cpp patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.h patching file Source/WebKit/WebProcess/Storage/WebSWContextManagerConnection.messages.in patching file Source/WebKit/WebProcess/WebProcess.cpp patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm patching file Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.h patching file Tools/WebKitTestRunner/TestController.cpp patching file Tools/WebKitTestRunner/TestInvocation.cpp patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/http/tests/workers/service/resources/postmessage-after-sw-process-crash.js patching file LayoutTests/http/tests/workers/service/resources/postmessage-after-terminate.js patching file LayoutTests/http/tests/workers/service/resources/postmessage-after-terminating-hung-worker.js patching file LayoutTests/http/wpt/service-workers/online.https.html patching file LayoutTests/http/wpt/service-workers/service-worker-different-process.https-expected.txt patching file LayoutTests/http/wpt/service-workers/service-worker-different-process.https.html patching file LayoutTests/http/wpt/service-workers/service-worker-process-worker.js Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Dumez']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/13125926
Created attachment 380857 [details] Patch for landing
Comment on attachment 380857 [details] Patch for landing Clearing flags on attachment: 380857 Committed r251067: <https://trac.webkit.org/changeset/251067>
All reviewed patches have been landed. Closing bug.
This broke some GTK bots. Any idea? https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Debian%20Stable%20%28Build%29/builds/26075/steps/compile-webkit/logs/stdio
WebKit::WebProcessProxy::createForServiceWorkers(WebKit::WebProcessPool&, WebCore::RegistrableDomain&&, WebKit::WebsiteDataStore&): error: undefined reference to 'WebKit::WebProcessProxy::enableServiceWorkers()' collect2: error: ld returned 1 exit status
(In reply to Philippe Normand from comment #26) > WebKit::WebProcessProxy::createForServiceWorkers(WebKit::WebProcessPool&, > WebCore::RegistrableDomain&&, WebKit::WebsiteDataStore&): error: undefined > reference to 'WebKit::WebProcessProxy::enableServiceWorkers()' > collect2: error: ld returned 1 exit status Oh probably ENABLE(SERVICE_WORKER) issue, I'll fix it.
Reopening to attach new patch.
Created attachment 380875 [details] Patch
<rdar://problem/56245134>
<rdar://problem/56245136>
Comment on attachment 380875 [details] Patch rs=me :)
Comment on attachment 380875 [details] Patch Clearing flags on attachment: 380875 Committed r251073: <https://trac.webkit.org/changeset/251073>