Bug 214079 – [Cocoa] Make it possible to establish direct XPC connections between WebKit processes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

In order to be able to share XPC objects with other WebKit processes, it should be possible to establish direct XPC connections between WebKit processes. See https://bugs.webkit.org/show_bug.cgi?id=213794 for an example of when this is needed.

[Per Arne Vollan]

Created attachment 403777 [details]
Patch

[Brent Fulgham]

Comment on attachment 403777 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403777&action=review

r=me

> Source/WTF/ChangeLog:3
> +        [Cocoa] Update Launch Services database in the WebContent process from the Network process

Title should probably be: Make it possible to establish direct XPC connections between WebKit processes

> Source/WTF/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=213794

This should be 214079

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=213794

Same comments as above.

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=213794

Ditto.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 403777 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403777&action=review
> 
> r=me
> 
> > Source/WTF/ChangeLog:3
> > +        [Cocoa] Update Launch Services database in the WebContent process from the Network process
> 
> Title should probably be: Make it possible to establish direct XPC
> connections between WebKit processes
> 
> > Source/WTF/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=213794
> 
> This should be 214079
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=213794
> 
> Same comments as above.
> 
> > Tools/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=213794
> 
> Ditto.

Will fix.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 403799 [details]
Patch

[EWS]

Committed r264128: <https://trac.webkit.org/changeset/264128>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403799 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65236537>

[Sam Weinig]

Comment on attachment 403799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403799&action=review

> Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> +class XPCEndpoint {
> +public:

Why is this class in WebCore? In what scenarios do we see WebCore needing to use XPC endpoints directly?

[Per Arne Vollan]

(In reply to Sam Weinig from comment #7)
> Comment on attachment 403799 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> 
> > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > +class XPCEndpoint {
> > +public:
> 
> Why is this class in WebCore? In what scenarios do we see WebCore needing to
> use XPC endpoints directly?

I moved this to WebCore to be able to unit test the classes by exporting symbols with WEBCORE_EXPORT. But perhaps there is another define for exporting symbols from WebKit?

[Sam Weinig]

(In reply to Per Arne Vollan from comment #8)
> (In reply to Sam Weinig from comment #7)
> > Comment on attachment 403799 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > 
> > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > +class XPCEndpoint {
> > > +public:
> > 
> > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > use XPC endpoints directly?
> 
> I moved this to WebCore to be able to unit test the classes by exporting
> symbols with WEBCORE_EXPORT. But perhaps there is another define for
> exporting symbols from WebKit?

Yeah, that's what WK_EXPORT does.

I am also not certain that XPCEndpoint really needs a wrapper class, it really doesn't expose enough API for it to seem warranted. We generally don't wrap classes just because, but rather to create abstractions for multiple ports, and that doesn't apply here. It seems like using OSObjectPtr<xpc_endpoint_t> is likely fine.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #9)
> (In reply to Per Arne Vollan from comment #8)
> > (In reply to Sam Weinig from comment #7)
> > > Comment on attachment 403799 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > 
> > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > +class XPCEndpoint {
> > > > +public:
> > > 
> > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > use XPC endpoints directly?
> > 
> > I moved this to WebCore to be able to unit test the classes by exporting
> > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > exporting symbols from WebKit?
> 
> Yeah, that's what WK_EXPORT does.
> 

Ah, I see! I will move the classes back to WebKit.

> I am also not certain that XPCEndpoint really needs a wrapper class, it
> really doesn't expose enough API for it to seem warranted. We generally
> don't wrap classes just because, but rather to create abstractions for
> multiple ports, and that doesn't apply here. It seems like using
> OSObjectPtr<xpc_endpoint_t> is likely fine.

I actually intended for them to be more than just wrappers. There is additional code in these classes to create and establish the XPC connection with the endpoint, as well as checking that the other end really is a WebKit process before initiating communication. Perhaps they would benefit from being renamed?

[Sam Weinig]

(In reply to Per Arne Vollan from comment #10)
> (In reply to Sam Weinig from comment #9)
> > (In reply to Per Arne Vollan from comment #8)
> > > (In reply to Sam Weinig from comment #7)
> > > > Comment on attachment 403799 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > > 
> > > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > > +class XPCEndpoint {
> > > > > +public:
> > > > 
> > > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > > use XPC endpoints directly?
> > > 
> > > I moved this to WebCore to be able to unit test the classes by exporting
> > > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > > exporting symbols from WebKit?
> > 
> > Yeah, that's what WK_EXPORT does.
> > 
> 
> Ah, I see! I will move the classes back to WebKit.
> 
> > I am also not certain that XPCEndpoint really needs a wrapper class, it
> > really doesn't expose enough API for it to seem warranted. We generally
> > don't wrap classes just because, but rather to create abstractions for
> > multiple ports, and that doesn't apply here. It seems like using
> > OSObjectPtr<xpc_endpoint_t> is likely fine.
> 
> I actually intended for them to be more than just wrappers. There is
> additional code in these classes to create and establish the XPC connection
> with the endpoint, as well as checking that the other end really is a WebKit
> process before initiating communication. Perhaps they would benefit from
> being renamed?

Yes, if you intend for them to be more than what xpc_endpoint_t's do, I think we are better off with a different name. Otherwise, it will be quite confusing.

Can you explain a bit more about what the use case here is? I am really not sure this is the right way to go. Adding more IPC primitives is something we should do very carefully. It's really easy to get this stuff wrong.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #11)
> (In reply to Per Arne Vollan from comment #10)
> > (In reply to Sam Weinig from comment #9)
> > > (In reply to Per Arne Vollan from comment #8)
> > > > (In reply to Sam Weinig from comment #7)
> > > > > Comment on attachment 403799 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > > > 
> > > > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > > > +class XPCEndpoint {
> > > > > > +public:
> > > > > 
> > > > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > > > use XPC endpoints directly?
> > > > 
> > > > I moved this to WebCore to be able to unit test the classes by exporting
> > > > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > > > exporting symbols from WebKit?
> > > 
> > > Yeah, that's what WK_EXPORT does.
> > > 
> > 
> > Ah, I see! I will move the classes back to WebKit.
> > 
> > > I am also not certain that XPCEndpoint really needs a wrapper class, it
> > > really doesn't expose enough API for it to seem warranted. We generally
> > > don't wrap classes just because, but rather to create abstractions for
> > > multiple ports, and that doesn't apply here. It seems like using
> > > OSObjectPtr<xpc_endpoint_t> is likely fine.
> > 
> > I actually intended for them to be more than just wrappers. There is
> > additional code in these classes to create and establish the XPC connection
> > with the endpoint, as well as checking that the other end really is a WebKit
> > process before initiating communication. Perhaps they would benefit from
> > being renamed?
> 
> Yes, if you intend for them to be more than what xpc_endpoint_t's do, I
> think we are better off with a different name. Otherwise, it will be quite
> confusing.
> 
> Can you explain a bit more about what the use case here is? I am really not
> sure this is the right way to go. Adding more IPC primitives is something we
> should do very carefully. It's really easy to get this stuff wrong.

Yes, the use case is to send the Launch Services database represented in an XPC object from the Networking process to the WebContent process; see https://bugs.webkit.org/show_bug.cgi?id=213794. The database is only available as an XPC object, so it cannot be sent over the existing CoreIPC connection between the Networking and WebContent process. The Networking process was chosen as the source, since we are controlling its sandbox, making sure that the database is available for sending in the Networking process.

[Sam Weinig]

(In reply to Per Arne Vollan from comment #12)
> (In reply to Sam Weinig from comment #11)
> > (In reply to Per Arne Vollan from comment #10)
> > > (In reply to Sam Weinig from comment #9)
> > > > (In reply to Per Arne Vollan from comment #8)
> > > > > (In reply to Sam Weinig from comment #7)
> > > > > > Comment on attachment 403799 [details]
> > > > > > Patch
> > > > > > 
> > > > > > View in context:
> > > > > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > > > > 
> > > > > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > > > > +class XPCEndpoint {
> > > > > > > +public:
> > > > > > 
> > > > > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > > > > use XPC endpoints directly?
> > > > > 
> > > > > I moved this to WebCore to be able to unit test the classes by exporting
> > > > > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > > > > exporting symbols from WebKit?
> > > > 
> > > > Yeah, that's what WK_EXPORT does.
> > > > 
> > > 
> > > Ah, I see! I will move the classes back to WebKit.
> > > 
> > > > I am also not certain that XPCEndpoint really needs a wrapper class, it
> > > > really doesn't expose enough API for it to seem warranted. We generally
> > > > don't wrap classes just because, but rather to create abstractions for
> > > > multiple ports, and that doesn't apply here. It seems like using
> > > > OSObjectPtr<xpc_endpoint_t> is likely fine.
> > > 
> > > I actually intended for them to be more than just wrappers. There is
> > > additional code in these classes to create and establish the XPC connection
> > > with the endpoint, as well as checking that the other end really is a WebKit
> > > process before initiating communication. Perhaps they would benefit from
> > > being renamed?
> > 
> > Yes, if you intend for them to be more than what xpc_endpoint_t's do, I
> > think we are better off with a different name. Otherwise, it will be quite
> > confusing.
> > 
> > Can you explain a bit more about what the use case here is? I am really not
> > sure this is the right way to go. Adding more IPC primitives is something we
> > should do very carefully. It's really easy to get this stuff wrong.
> 
> Yes, the use case is to send the Launch Services database represented in an
> XPC object from the Networking process to the WebContent process; see
> https://bugs.webkit.org/show_bug.cgi?id=213794. The database is only
> available as an XPC object, so it cannot be sent over the existing CoreIPC
> connection between the Networking and WebContent process. The Networking
> process was chosen as the source, since we are controlling its sandbox,
> making sure that the database is available for sending in the Networking
> process.

I'm trying to follow the logic in that other patch, but it's pretty gnarly. It seems like it sets things up so, at least in the web process, messages are coming in both over the mach port and over the xpc connection.

It really seems like what this is asking for is to just switch to using xpc messaging for CoreIPC (e.g. each CoreIPC message on Darwin platforms gets encoded as one xpc_data, and not using xpc_object encoding in the more traditional sense). 

I would really like you to reconsider the amount of complexity this current system you have written adds, and whether it is the right trade off. I think if you spend a bit more time working through the design, you could come up with something much cleaner, that doesn't add new primitives and doesn't add platform specific code into platform agnostic classes.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #13)
> (In reply to Per Arne Vollan from comment #12)
> > (In reply to Sam Weinig from comment #11)
> > > (In reply to Per Arne Vollan from comment #10)
> > > > (In reply to Sam Weinig from comment #9)
> > > > > (In reply to Per Arne Vollan from comment #8)
> > > > > > (In reply to Sam Weinig from comment #7)
> > > > > > > Comment on attachment 403799 [details]
> > > > > > > Patch
> > > > > > > 
> > > > > > > View in context:
> > > > > > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > > > > > 
> > > > > > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > > > > > +class XPCEndpoint {
> > > > > > > > +public:
> > > > > > > 
> > > > > > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > > > > > use XPC endpoints directly?
> > > > > > 
> > > > > > I moved this to WebCore to be able to unit test the classes by exporting
> > > > > > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > > > > > exporting symbols from WebKit?
> > > > > 
> > > > > Yeah, that's what WK_EXPORT does.
> > > > > 
> > > > 
> > > > Ah, I see! I will move the classes back to WebKit.
> > > > 
> > > > > I am also not certain that XPCEndpoint really needs a wrapper class, it
> > > > > really doesn't expose enough API for it to seem warranted. We generally
> > > > > don't wrap classes just because, but rather to create abstractions for
> > > > > multiple ports, and that doesn't apply here. It seems like using
> > > > > OSObjectPtr<xpc_endpoint_t> is likely fine.
> > > > 
> > > > I actually intended for them to be more than just wrappers. There is
> > > > additional code in these classes to create and establish the XPC connection
> > > > with the endpoint, as well as checking that the other end really is a WebKit
> > > > process before initiating communication. Perhaps they would benefit from
> > > > being renamed?
> > > 
> > > Yes, if you intend for them to be more than what xpc_endpoint_t's do, I
> > > think we are better off with a different name. Otherwise, it will be quite
> > > confusing.
> > > 
> > > Can you explain a bit more about what the use case here is? I am really not
> > > sure this is the right way to go. Adding more IPC primitives is something we
> > > should do very carefully. It's really easy to get this stuff wrong.
> > 
> > Yes, the use case is to send the Launch Services database represented in an
> > XPC object from the Networking process to the WebContent process; see
> > https://bugs.webkit.org/show_bug.cgi?id=213794. The database is only
> > available as an XPC object, so it cannot be sent over the existing CoreIPC
> > connection between the Networking and WebContent process. The Networking
> > process was chosen as the source, since we are controlling its sandbox,
> > making sure that the database is available for sending in the Networking
> > process.
> 
> I'm trying to follow the logic in that other patch, but it's pretty gnarly.
> It seems like it sets things up so, at least in the web process, messages
> are coming in both over the mach port and over the xpc connection.
> 
> It really seems like what this is asking for is to just switch to using xpc
> messaging for CoreIPC (e.g. each CoreIPC message on Darwin platforms gets
> encoded as one xpc_data, and not using xpc_object encoding in the more
> traditional sense). 
> 
> I would really like you to reconsider the amount of complexity this current
> system you have written adds, and whether it is the right trade off. I think
> if you spend a bit more time working through the design, you could come up
> with something much cleaner, that doesn't add new primitives and doesn't add
> platform specific code into platform agnostic classes.

That it a good point. So if I understand you correctly, we should aim to replace the current CoreIPC connection between the Networking process and the WebContent process with an XPC connection? I think that makes sense, since it will allow us to send XPC objects over this connection. Are there some performance benefits to a low level CoreIPC connection? Or security benefits?

Thanks for reviewing!

[Sam Weinig]

(In reply to Per Arne Vollan from comment #14)
> (In reply to Sam Weinig from comment #13)
> > (In reply to Per Arne Vollan from comment #12)
> > > (In reply to Sam Weinig from comment #11)
> > > > (In reply to Per Arne Vollan from comment #10)
> > > > > (In reply to Sam Weinig from comment #9)
> > > > > > (In reply to Per Arne Vollan from comment #8)
> > > > > > > (In reply to Sam Weinig from comment #7)
> > > > > > > > Comment on attachment 403799 [details]
> > > > > > > > Patch
> > > > > > > > 
> > > > > > > > View in context:
> > > > > > > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > > > > > > 
> > > > > > > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > > > > > > +class XPCEndpoint {
> > > > > > > > > +public:
> > > > > > > > 
> > > > > > > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > > > > > > use XPC endpoints directly?
> > > > > > > 
> > > > > > > I moved this to WebCore to be able to unit test the classes by exporting
> > > > > > > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > > > > > > exporting symbols from WebKit?
> > > > > > 
> > > > > > Yeah, that's what WK_EXPORT does.
> > > > > > 
> > > > > 
> > > > > Ah, I see! I will move the classes back to WebKit.
> > > > > 
> > > > > > I am also not certain that XPCEndpoint really needs a wrapper class, it
> > > > > > really doesn't expose enough API for it to seem warranted. We generally
> > > > > > don't wrap classes just because, but rather to create abstractions for
> > > > > > multiple ports, and that doesn't apply here. It seems like using
> > > > > > OSObjectPtr<xpc_endpoint_t> is likely fine.
> > > > > 
> > > > > I actually intended for them to be more than just wrappers. There is
> > > > > additional code in these classes to create and establish the XPC connection
> > > > > with the endpoint, as well as checking that the other end really is a WebKit
> > > > > process before initiating communication. Perhaps they would benefit from
> > > > > being renamed?
> > > > 
> > > > Yes, if you intend for them to be more than what xpc_endpoint_t's do, I
> > > > think we are better off with a different name. Otherwise, it will be quite
> > > > confusing.
> > > > 
> > > > Can you explain a bit more about what the use case here is? I am really not
> > > > sure this is the right way to go. Adding more IPC primitives is something we
> > > > should do very carefully. It's really easy to get this stuff wrong.
> > > 
> > > Yes, the use case is to send the Launch Services database represented in an
> > > XPC object from the Networking process to the WebContent process; see
> > > https://bugs.webkit.org/show_bug.cgi?id=213794. The database is only
> > > available as an XPC object, so it cannot be sent over the existing CoreIPC
> > > connection between the Networking and WebContent process. The Networking
> > > process was chosen as the source, since we are controlling its sandbox,
> > > making sure that the database is available for sending in the Networking
> > > process.
> > 
> > I'm trying to follow the logic in that other patch, but it's pretty gnarly.
> > It seems like it sets things up so, at least in the web process, messages
> > are coming in both over the mach port and over the xpc connection.
> > 
> > It really seems like what this is asking for is to just switch to using xpc
> > messaging for CoreIPC (e.g. each CoreIPC message on Darwin platforms gets
> > encoded as one xpc_data, and not using xpc_object encoding in the more
> > traditional sense). 
> > 
> > I would really like you to reconsider the amount of complexity this current
> > system you have written adds, and whether it is the right trade off. I think
> > if you spend a bit more time working through the design, you could come up
> > with something much cleaner, that doesn't add new primitives and doesn't add
> > platform specific code into platform agnostic classes.
> 
> That it a good point. So if I understand you correctly, we should aim to
> replace the current CoreIPC connection between the Networking process and
> the WebContent process with an XPC connection? I think that makes sense,
> since it will allow us to send XPC objects over this connection. Are there
> some performance benefits to a low level CoreIPC connection? Or security
> benefits?
> 
> Thanks for reviewing!

I can't remember all the details, but if memory and a little digging serve me, the way we establish a connection between the network process and WebContent processes is by passing a IPC::Connection::Identifier (which contains a machport) to the WebContent process (this is in the sync message from the WebProcess to the UIProcess called Messages::WebProcessProxy::GetNetworkProcessConnection(). The sync thing is another whole issue for another day).

If we used xpc as the underlying transport, rather than just for bootstrapping our machports, we could replace the machport in IPC::Connection::Identifier with an xpc_endpoint_t, and establish the connection by creating a new CoreIPC::Connection from the endpoint, rather than the machport.

As for you questions about benefits / tradeoffs, the tradeoff is a little less flexibility (you can play a lot of tricks using machports directly) and potentially a tiny bit of performance, but that would have to be measured, I can't actually be sure of that.

What we would gain quite a bit. We would be able to interact with system in a more coherent way, working better with scheduling and boosting, of which WebKit currently does a pretty hacky job. We would also be able to send things like xpc_enpoints directly, rather than requiring a side channel connection for them.

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #14)
> (In reply to Sam Weinig from comment #13)
> > (In reply to Per Arne Vollan from comment #12)
> > > (In reply to Sam Weinig from comment #11)
> > > > (In reply to Per Arne Vollan from comment #10)
> > > > > (In reply to Sam Weinig from comment #9)
> > > > > > (In reply to Per Arne Vollan from comment #8)
> > > > > > > (In reply to Sam Weinig from comment #7)
> > > > > > > > Comment on attachment 403799 [details]
> > > > > > > > Patch
> > > > > > > > 
> > > > > > > > View in context:
> > > > > > > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > > > > > > 
> > > > > > > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > > > > > > +class XPCEndpoint {
> > > > > > > > > +public:
> > > > > > > > 
> > > > > > > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > > > > > > use XPC endpoints directly?
> > > > > > > 
> > > > > > > I moved this to WebCore to be able to unit test the classes by exporting
> > > > > > > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > > > > > > exporting symbols from WebKit?
> > > > > > 
> > > > > > Yeah, that's what WK_EXPORT does.
> > > > > > 
> > > > > 
> > > > > Ah, I see! I will move the classes back to WebKit.
> > > > > 
> > > > > > I am also not certain that XPCEndpoint really needs a wrapper class, it
> > > > > > really doesn't expose enough API for it to seem warranted. We generally
> > > > > > don't wrap classes just because, but rather to create abstractions for
> > > > > > multiple ports, and that doesn't apply here. It seems like using
> > > > > > OSObjectPtr<xpc_endpoint_t> is likely fine.
> > > > > 
> > > > > I actually intended for them to be more than just wrappers. There is
> > > > > additional code in these classes to create and establish the XPC connection
> > > > > with the endpoint, as well as checking that the other end really is a WebKit
> > > > > process before initiating communication. Perhaps they would benefit from
> > > > > being renamed?
> > > > 
> > > > Yes, if you intend for them to be more than what xpc_endpoint_t's do, I
> > > > think we are better off with a different name. Otherwise, it will be quite
> > > > confusing.
> > > > 
> > > > Can you explain a bit more about what the use case here is? I am really not
> > > > sure this is the right way to go. Adding more IPC primitives is something we
> > > > should do very carefully. It's really easy to get this stuff wrong.
> > > 
> > > Yes, the use case is to send the Launch Services database represented in an
> > > XPC object from the Networking process to the WebContent process; see
> > > https://bugs.webkit.org/show_bug.cgi?id=213794. The database is only
> > > available as an XPC object, so it cannot be sent over the existing CoreIPC
> > > connection between the Networking and WebContent process. The Networking
> > > process was chosen as the source, since we are controlling its sandbox,
> > > making sure that the database is available for sending in the Networking
> > > process.
> > 
> > I'm trying to follow the logic in that other patch, but it's pretty gnarly.
> > It seems like it sets things up so, at least in the web process, messages
> > are coming in both over the mach port and over the xpc connection.
> > 
> > It really seems like what this is asking for is to just switch to using xpc
> > messaging for CoreIPC (e.g. each CoreIPC message on Darwin platforms gets
> > encoded as one xpc_data, and not using xpc_object encoding in the more
> > traditional sense). 
> > 
> > I would really like you to reconsider the amount of complexity this current
> > system you have written adds, and whether it is the right trade off. I think
> > if you spend a bit more time working through the design, you could come up
> > with something much cleaner, that doesn't add new primitives and doesn't add
> > platform specific code into platform agnostic classes.
> 
> That it a good point. So if I understand you correctly, we should aim to
> replace the current CoreIPC connection between the Networking process and
> the WebContent process with an XPC connection? I think that makes sense,
> since it will allow us to send XPC objects over this connection. Are there
> some performance benefits to a low level CoreIPC connection? Or security
> benefits?
> 
> Thanks for reviewing!

(In reply to Sam Weinig from comment #15)
> (In reply to Per Arne Vollan from comment #14)
> > (In reply to Sam Weinig from comment #13)
> > > (In reply to Per Arne Vollan from comment #12)
> > > > (In reply to Sam Weinig from comment #11)
> > > > > (In reply to Per Arne Vollan from comment #10)
> > > > > > (In reply to Sam Weinig from comment #9)
> > > > > > > (In reply to Per Arne Vollan from comment #8)
> > > > > > > > (In reply to Sam Weinig from comment #7)
> > > > > > > > > Comment on attachment 403799 [details]
> > > > > > > > > Patch
> > > > > > > > > 
> > > > > > > > > View in context:
> > > > > > > > > https://bugs.webkit.org/attachment.cgi?id=403799&action=review
> > > > > > > > > 
> > > > > > > > > > Source/WebCore/platform/cocoa/XPCEndpoint.h:34
> > > > > > > > > > +class XPCEndpoint {
> > > > > > > > > > +public:
> > > > > > > > > 
> > > > > > > > > Why is this class in WebCore? In what scenarios do we see WebCore needing to
> > > > > > > > > use XPC endpoints directly?
> > > > > > > > 
> > > > > > > > I moved this to WebCore to be able to unit test the classes by exporting
> > > > > > > > symbols with WEBCORE_EXPORT. But perhaps there is another define for
> > > > > > > > exporting symbols from WebKit?
> > > > > > > 
> > > > > > > Yeah, that's what WK_EXPORT does.
> > > > > > > 
> > > > > > 
> > > > > > Ah, I see! I will move the classes back to WebKit.
> > > > > > 
> > > > > > > I am also not certain that XPCEndpoint really needs a wrapper class, it
> > > > > > > really doesn't expose enough API for it to seem warranted. We generally
> > > > > > > don't wrap classes just because, but rather to create abstractions for
> > > > > > > multiple ports, and that doesn't apply here. It seems like using
> > > > > > > OSObjectPtr<xpc_endpoint_t> is likely fine.
> > > > > > 
> > > > > > I actually intended for them to be more than just wrappers. There is
> > > > > > additional code in these classes to create and establish the XPC connection
> > > > > > with the endpoint, as well as checking that the other end really is a WebKit
> > > > > > process before initiating communication. Perhaps they would benefit from
> > > > > > being renamed?
> > > > > 
> > > > > Yes, if you intend for them to be more than what xpc_endpoint_t's do, I
> > > > > think we are better off with a different name. Otherwise, it will be quite
> > > > > confusing.
> > > > > 
> > > > > Can you explain a bit more about what the use case here is? I am really not
> > > > > sure this is the right way to go. Adding more IPC primitives is something we
> > > > > should do very carefully. It's really easy to get this stuff wrong.
> > > > 
> > > > Yes, the use case is to send the Launch Services database represented in an
> > > > XPC object from the Networking process to the WebContent process; see
> > > > https://bugs.webkit.org/show_bug.cgi?id=213794. The database is only
> > > > available as an XPC object, so it cannot be sent over the existing CoreIPC
> > > > connection between the Networking and WebContent process. The Networking
> > > > process was chosen as the source, since we are controlling its sandbox,
> > > > making sure that the database is available for sending in the Networking
> > > > process.
> > > 
> > > I'm trying to follow the logic in that other patch, but it's pretty gnarly.
> > > It seems like it sets things up so, at least in the web process, messages
> > > are coming in both over the mach port and over the xpc connection.
> > > 
> > > It really seems like what this is asking for is to just switch to using xpc
> > > messaging for CoreIPC (e.g. each CoreIPC message on Darwin platforms gets
> > > encoded as one xpc_data, and not using xpc_object encoding in the more
> > > traditional sense). 
> > > 
> > > I would really like you to reconsider the amount of complexity this current
> > > system you have written adds, and whether it is the right trade off. I think
> > > if you spend a bit more time working through the design, you could come up
> > > with something much cleaner, that doesn't add new primitives and doesn't add
> > > platform specific code into platform agnostic classes.
> > 
> > That it a good point. So if I understand you correctly, we should aim to
> > replace the current CoreIPC connection between the Networking process and
> > the WebContent process with an XPC connection? I think that makes sense,
> > since it will allow us to send XPC objects over this connection. Are there
> > some performance benefits to a low level CoreIPC connection? Or security
> > benefits?
> > 
> > Thanks for reviewing!
> 
> I can't remember all the details, but if memory and a little digging serve
> me, the way we establish a connection between the network process and
> WebContent processes is by passing a IPC::Connection::Identifier (which
> contains a machport) to the WebContent process (this is in the sync message
> from the WebProcess to the UIProcess called
> Messages::WebProcessProxy::GetNetworkProcessConnection(). The sync thing is
> another whole issue for another day).
> 
> If we used xpc as the underlying transport, rather than just for
> bootstrapping our machports, we could replace the machport in
> IPC::Connection::Identifier with an xpc_endpoint_t, and establish the
> connection by creating a new CoreIPC::Connection from the endpoint, rather
> than the machport.
> 
> As for you questions about benefits / tradeoffs, the tradeoff is a little
> less flexibility (you can play a lot of tricks using machports directly) and
> potentially a tiny bit of performance, but that would have to be measured, I
> can't actually be sure of that.
> 
> What we would gain quite a bit. We would be able to interact with system in
> a more coherent way, working better with scheduling and boosting, of which
> WebKit currently does a pretty hacky job. We would also be able to send
> things like xpc_enpoints directly, rather than requiring a side channel
> connection for them.

Sounds good. We could also stop creating CoreIPC connections between the UI process and the Networking/WebContent process. We could just reuse the bootstrap XPC connection. I think we should do it :)