Use WebProcess processIdentifier to identify Service Worker connections
Created attachment 378055 [details] Patch
Created attachment 378064 [details] Patch
Created attachment 378067 [details] Patch
Created attachment 378068 [details] Patch
Created attachment 378073 [details] Patch
Created attachment 378078 [details] Patch
Created attachment 378180 [details] Patch
Comment on attachment 378180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378180&action=review > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:324 > + HashMap<uint64_t, WeakPtr<WebSWServerConnection>> m_swConnections; Going from a strong identifier type to uint64_t seems like a pretty bad regression for me :/
(In reply to Chris Dumez from comment #8) > Comment on attachment 378180 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378180&action=review > > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:324 > > + HashMap<uint64_t, WeakPtr<WebSWServerConnection>> m_swConnections; > > Going from a strong identifier type to uint64_t seems like a pretty bad > regression for me :/ This is not a regression. This map is an internal detail of NetworkConnectionToWebProcess that is not exposed anywhere else. It is used to get a connection from a decoderId which is a uint64_t. I can make the map take a PAL::SessionID, in which case I will need to add a way to create a PAL::SessionID from a uint64_t (similarly to makeObjectIdentifier). I thought this was not really needed but can change the patch accordingly, this might improve the readability of the code.
(In reply to youenn fablet from comment #9) > (In reply to Chris Dumez from comment #8) > > Comment on attachment 378180 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=378180&action=review > > > > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:324 > > > + HashMap<uint64_t, WeakPtr<WebSWServerConnection>> m_swConnections; > > > > Going from a strong identifier type to uint64_t seems like a pretty bad > > regression for me :/ > > This is not a regression. > This map is an internal detail of NetworkConnectionToWebProcess that is not > exposed anywhere else. > It is used to get a connection from a decoderId which is a uint64_t. > I can make the map take a PAL::SessionID, in which case I will need to add a > way to create a PAL::SessionID from a uint64_t (similarly to > makeObjectIdentifier). > I thought this was not really needed but can change the patch accordingly, > this might improve the readability of the code. If it is really a sessionID then I think it should use the PAL::SessionID type, even if it means converting IPC ids to strong identifiers in a couple of places. Strong identifier types are safer and improve readability a lot.
Created attachment 378201 [details] Patch
Comment on attachment 378201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378201&action=review > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:65 > + : SWServer::Connection(server, makeObjectIdentifier<SWServerConnectionIdentifierType>(processIdentifier.toUInt64())) This is very weird, seems like terrible practice to convert one ID type to another. > Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:60 > + , m_identifier(makeObjectIdentifier<SWServerConnectionIdentifierType>(Process::identifier().toUInt64())) ditto.
Created attachment 378359 [details] Patch
Comment on attachment 378359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378359&action=review > Source/WebCore/ChangeLog:3 > + Use WebProcess processIdentifier to identify Service Worker connections This change log does not explain why we're making this change, what was used before to identify the connections and why the new approach is better? > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:324 > + HashMap<PAL::SessionID, WeakPtr<WebSWServerConnection>> m_swConnections; It does not look to me that we're going in the right direction. We want 1 sessionID per WebContent process so doing to a HashMap of SessionID on the NetworkConnectionToWebProcess does not make sense to me.
(In reply to Chris Dumez from comment #14) > Comment on attachment 378359 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378359&action=review > > > Source/WebCore/ChangeLog:3 > > + Use WebProcess processIdentifier to identify Service Worker connections > > This change log does not explain why we're making this change, what was used > before to identify the connections and why the new approach is better? The change log says that these IDs are stable over network process crash. This makes the network process crash handling easier and more robust on WebProcess side. The change log also says that this removes a sync IPC. We generally want to avoid sync IPC. > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:324 > > + HashMap<PAL::SessionID, WeakPtr<WebSWServerConnection>> m_swConnections; > > It does not look to me that we're going in the right direction. We want 1 > sessionID per WebContent process so doing to a HashMap of SessionID on the > NetworkConnectionToWebProcess does not make sense to me. We need this map to preserve status quo and make sure service workers are working properly. This patch is actually helping if we want to do the simplification of one sessionID per web process. We will just have to go from two sessionID-based maps (one in web process and one in network process) to singletons. This should be very straightforward compared to the current code base where we have 3 maps, two of them being not keyed by session IDs.
Third benefit in change log I forgot to mention here is the changes to WebSWServerToContextConnection which will make it possible to run service workers in web processes that also run pages.
(In reply to youenn fablet from comment #15) > (In reply to Chris Dumez from comment #14) > > Comment on attachment 378359 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=378359&action=review > > > > > Source/WebCore/ChangeLog:3 > > > + Use WebProcess processIdentifier to identify Service Worker connections > > > > This change log does not explain why we're making this change, what was used > > before to identify the connections and why the new approach is better? > > The change log says that these IDs are stable over network process crash. > This makes the network process crash handling easier and more robust on > WebProcess side. > > The change log also says that this removes a sync IPC. > We generally want to avoid sync IPC. > > > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:324 > > > + HashMap<PAL::SessionID, WeakPtr<WebSWServerConnection>> m_swConnections; > > > > It does not look to me that we're going in the right direction. We want 1 > > sessionID per WebContent process so doing to a HashMap of SessionID on the > > NetworkConnectionToWebProcess does not make sense to me. > > We need this map to preserve status quo and make sure service workers are > working properly. > > This patch is actually helping if we want to do the simplification of one > sessionID per web process. We will just have to go from two sessionID-based > maps (one in web process and one in network process) to singletons. This > should be very straightforward compared to the current code base where we > have 3 maps, two of them being not keyed by session IDs. I do still have concerns about refactoring code that is wrong (before fixing what is wrong about it) and potentially making it harder to fix the bug by reinforcing the bad design. My preference would be to first stop sharing service worker processes for sessionIDs and then refactor. Having a HashMap of sessionIDs on a connection to the webProcess looks very wrong.
(In reply to Chris Dumez from comment #17) > (In reply to youenn fablet from comment #15) > > (In reply to Chris Dumez from comment #14) > > > Comment on attachment 378359 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=378359&action=review > > > > > > > Source/WebCore/ChangeLog:3 > > > > + Use WebProcess processIdentifier to identify Service Worker connections > > > > > > This change log does not explain why we're making this change, what was used > > > before to identify the connections and why the new approach is better? > > > > The change log says that these IDs are stable over network process crash. > > This makes the network process crash handling easier and more robust on > > WebProcess side. > > > > The change log also says that this removes a sync IPC. > > We generally want to avoid sync IPC. > > > > > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:324 > > > > + HashMap<PAL::SessionID, WeakPtr<WebSWServerConnection>> m_swConnections; > > > > > > It does not look to me that we're going in the right direction. We want 1 > > > sessionID per WebContent process so doing to a HashMap of SessionID on the > > > NetworkConnectionToWebProcess does not make sense to me. > > > > We need this map to preserve status quo and make sure service workers are > > working properly. > > > > This patch is actually helping if we want to do the simplification of one > > sessionID per web process. We will just have to go from two sessionID-based > > maps (one in web process and one in network process) to singletons. This > > should be very straightforward compared to the current code base where we > > have 3 maps, two of them being not keyed by session IDs. > > I do still have concerns about refactoring code that is wrong (before fixing > what is wrong about it) and potentially making it harder to fix the bug by > reinforcing the bad design. > My preference would be to first stop sharing service worker processes for > sessionIDs and then refactor. Having a HashMap of sessionIDs on a connection > to the webProcess looks very wrong. And by. the way, I do not mind doing the work for having separate service worker processes per session if you'd like. The only reason I have not done so last week is because this patch is blocking me.
> I do still have concerns about refactoring code that is wrong (before fixing > what is wrong about it) and potentially making it harder to fix the bug by > reinforcing the bad design. I don't think there is consensus yet to stop sharing web processes for service worker with different sessionIDs. This is the current implementation, it works and there are some known perf benefits. Agreed there are some potential privacy risks, that would be nice to fix. To fix this bug, the patch will mostly change UIProcess code, not code doing WebProcess/NetworkProcess communication, which is the part you seem to be against. So this part of the patch is not making it harder to fix it in any way. In fact, the is actually getting us closer to the fix. The current code base is not always giving the session ID when creating a service worker process. With the patch, the service worker process is always launched with a known session ID (see WebProcessPool::establishWorkerContextConnectionToNetworkProcess). This patch is also helpful for simplifications to the code base when service workers are sessionID partitioned. > My preference would be to first stop sharing service worker processes for > sessionIDs and then refactor. Having a HashMap of sessionIDs on a connection > to the webProcess looks very wrong. I think we should continue with the plan to run service workers in existing web processes that run pages. Once done (and we are not too far), the bug you mention will be fixed.
(In reply to youenn fablet from comment #19) > > I do still have concerns about refactoring code that is wrong (before fixing > > what is wrong about it) and potentially making it harder to fix the bug by > > reinforcing the bad design. > > I don't think there is consensus yet to stop sharing web processes for > service worker with different sessionIDs. This exact issue had to be fixed before shipping PSON, service workers are not any different. This has to be fixed asap.
(In reply to Chris Dumez from comment #20) > (In reply to youenn fablet from comment #19) > > > I do still have concerns about refactoring code that is wrong (before fixing > > > what is wrong about it) and potentially making it harder to fix the bug by > > > reinforcing the bad design. > > > > I don't think there is consensus yet to stop sharing web processes for > > service worker with different sessionIDs. > > This exact issue had to be fixed before shipping PSON, service workers are > not any different. This has to be fixed asap. + Geoff. Brady & Alex are already in cc and are also familiar with this matter.
(In reply to youenn fablet from comment #19) > > I do still have concerns about refactoring code that is wrong (before fixing > > what is wrong about it) and potentially making it harder to fix the bug by > > reinforcing the bad design. > > I don't think there is consensus yet to stop sharing web processes for > service worker with different sessionIDs. > This is the current implementation, it works and there are some known perf > benefits. Agreed there are some potential privacy risks, that would be nice > to fix. > To fix this bug, the patch will mostly change UIProcess code, not code doing > WebProcess/NetworkProcess communication, which is the part you seem to be > against. So this part of the patch is not making it harder to fix it in any > way. I had started to write that patch (split Service workers by sessionID) and had to stop when I saw your refactoring patch because we are clearly touching the same code. I also think the priority should be to fix our security / privacy issue before refactoring code to help with performance.
> I don't think there is consensus yet to stop sharing web processes for > service worker with different sessionIDs. > This is the current implementation, it works and there are some known perf > benefits. Agreed there are some potential privacy risks, that would be nice > to fix. I didn't read the rest of the discussion, but I want to comment on this detail: It is a Privacy requirement not to share the same web process for distinct sessionIDs. You can read more in <rdar://problem/48421064>.
Comment on attachment 378359 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378359&action=review r=me, let's split the sessions in a follow-up. > Source/WebCore/workers/service/ServiceWorkerTypes.h:75 > using SWServerConnectionIdentifier = ObjectIdentifier<SWServerConnectionIdentifierType>; Why isn't this simply using SWServerConnectionIdentifier = ProcessIdentifier; ? > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:298 > + sessionID = server.sessionID(); Seems unfortunate we'll keep iterating even though we've found the sessionID. > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:893 > + m_swContextConnection = WebSWServerToContextConnection::create(m_networkProcess, registrableDomain, m_connection.get()); WTFMove(registrableDomain) ? > Source/WebKit/UIProcess/WebProcessPool.cpp:699 > + WebsiteDataStore* websiteDataStore = WebsiteDataStore::existingNonDefaultDataStoreForSessionID(sessionID); auto*
(In reply to Chris Dumez from comment #24) > Comment on attachment 378359 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378359&action=review > > r=me, let's split the sessions in a follow-up. > > > Source/WebCore/workers/service/ServiceWorkerTypes.h:75 > > using SWServerConnectionIdentifier = ObjectIdentifier<SWServerConnectionIdentifierType>; > > Why isn't this simply using SWServerConnectionIdentifier = > ProcessIdentifier; ? There are some uses of makeObjectIdentifier<SWServerConnectionIdentifierType> We could rewrite them as makeObjectIdentifier<ProcessIdentifierType> but that seems a bit weird. > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:298 > > + sessionID = server.sessionID(); > > Seems unfortunate we'll keep iterating even though we've found the sessionID. We need to iterate other all SWServers, so that all related service worker states get updated. This code will go away with next patch (bug 201643). > > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:893 > > + m_swContextConnection = WebSWServerToContextConnection::create(m_networkProcess, registrableDomain, m_connection.get()); > > WTFMove(registrableDomain) ? This refactoring is done in next patch (bug 201643) where we touch the constructor. > > Source/WebKit/UIProcess/WebProcessPool.cpp:699 > > + WebsiteDataStore* websiteDataStore = WebsiteDataStore::existingNonDefaultDataStoreForSessionID(sessionID); > > auto* OK
Created attachment 378626 [details] Patch for landing
Comment on attachment 378626 [details] Patch for landing Clearing flags on attachment: 378626 Committed r249801: <https://trac.webkit.org/changeset/249801>
All reviewed patches have been landed. Closing bug.
<rdar://problem/55293963>
This change has caused http/tests/workers/service/postmessage-after-sw-process-crash.https.html to frequently fail and http/tests/workers/service/postmessage-after-terminate.https.html to frequently time out Mojave / iOS 12 Release WK2 bots --- /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/http/tests/workers/service/postmessage-after-sw-process-crash.https-expected.txt +++ /Volumes/Data/slave/ios-simulator-12-release-tests-wk2/build/layout-test-results/http/tests/workers/service/postmessage-after-sw-process-crash.https-actual.txt @@ -1,5 +1,5 @@ * Sending State to Service Worker * Asking Service Worker if it received the state * Simulating Service Worker process crash -PASS: service worker lost the state and responded the postMessage after process termination +FAIL: service worker did not respond after process termination https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fpostmessage-after-sw-process-crash.https.html%20http%2Ftests%2Fworkers%2Fservice%2Fpostmessage-after-terminate.https.html
Reverted r249801 for reason: Caused two servier worker layout tests to become flaky. Committed r249820: <https://trac.webkit.org/changeset/249820>
(In reply to Ryan Haddad from comment #31) > Reverted r249801 for reason: > > Caused two servier worker layout tests to become flaky. > > Committed r249820: <https://trac.webkit.org/changeset/249820> There was a race condition between destruction of the old context connection and the new one, which is screwing the connection map. I updated the patch to destroy the WebSWServerToContextConnection just after it gets closed.
Created attachment 378723 [details] Patch
Comment on attachment 378723 [details] Patch Clearing flags on attachment: 378723 Committed r249832: <https://trac.webkit.org/changeset/249832>