Bug 201459 – Use WebProcess processIdentifier to identify Service Worker connections

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

Use WebProcess processIdentifier to identify Service Worker connections

[youenn fablet]

Created attachment 378055 [details]
Patch

[youenn fablet]

Created attachment 378064 [details]
Patch

[youenn fablet]

Created attachment 378067 [details]
Patch

[youenn fablet]

Created attachment 378068 [details]
Patch

[youenn fablet]

Created attachment 378073 [details]
Patch

[youenn fablet]

Created attachment 378078 [details]
Patch

[youenn fablet]

Created attachment 378180 [details]
Patch

[Chris Dumez]

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 :/

[youenn fablet]

(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.

[Chris Dumez]

(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.

[youenn fablet]

Created attachment 378201 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

Created attachment 378359 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

(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.

[youenn fablet]

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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[youenn fablet]

> 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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Geoffrey Garen]

> 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>.

[Chris Dumez]

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*

[youenn fablet]

(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

[youenn fablet]

Created attachment 378626 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 378626 [details]
Patch for landing

Clearing flags on attachment: 378626

Committed r249801: <https://trac.webkit.org/changeset/249801>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55293963>

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r249801 for reason:

Caused two servier worker layout tests to become flaky.

Committed r249820: <https://trac.webkit.org/changeset/249820>

[youenn fablet]

(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.

[youenn fablet]

Created attachment 378723 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 378723 [details]
Patch

Clearing flags on attachment: 378723

Committed r249832: <https://trac.webkit.org/changeset/249832>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.