Bug 213794 – [Cocoa] Update Launch Services database in the WebContent process from the Network process

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

[Per Arne Vollan]

In order to be able to close the connection to the Launch Services database mapping service, we should update the Launch Services database in the WebContent process from the Network process. The Network process is chosen as the source, since the UI process does not always have access to the database mapping service.

[Per Arne Vollan]

Created attachment 403200 [details]
Patch

[Per Arne Vollan]

Created attachment 403205 [details]
Patch

[Per Arne Vollan]

Created attachment 403210 [details]
Patch

[Per Arne Vollan]

Created attachment 403211 [details]
Patch

[Per Arne Vollan]

Created attachment 403214 [details]
Patch

[Per Arne Vollan]

Created attachment 403220 [details]
Patch

[Per Arne Vollan]

Created attachment 403227 [details]
Patch

[Per Arne Vollan]

Created attachment 403232 [details]
Patch

[Brent Fulgham]

<rdar://problem/64168895>

[Per Arne Vollan]

Created attachment 403397 [details]
Patch

[Per Arne Vollan]

Created attachment 403399 [details]
Patch

[Per Arne Vollan]

Created attachment 403405 [details]
Patch

[Per Arne Vollan]

Created attachment 403414 [details]
Patch

[Per Arne Vollan]

Created attachment 403589 [details]
Patch

[Per Arne Vollan]

Created attachment 403592 [details]
Patch

[Per Arne Vollan]

Created attachment 403593 [details]
Patch

[Per Arne Vollan]

Created attachment 403595 [details]
Patch

[Per Arne Vollan]

Created attachment 403603 [details]
Patch

[Per Arne Vollan]

Created attachment 403614 [details]
Patch

[Brent Fulgham]

Comment on attachment 403614 [details]
Patch

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

I think this looks fine, but I had a few questions. I also think it would be good for Jiewen or someone else with XPC experience to take a look at the implementation.

> Source/WebKit/ChangeLog:9
> +        sent from the Networking process to the WebContent process, represented by a xpc object. The Networking process is chosen instead of the UI

"represented by AN xpc object." (and elsewhere in the summary).

> Source/WebKit/ChangeLog:22
> +        No new tests, since there already exist tests for this. UTI to MIME type mapping is being used in WebKit and underlying framework, and many

underlying frameworks, ...

> Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm:44
> +    auto context = [NSClassFromString(@"LSDatabaseContext") sharedDatabaseContext];

We seem to construct this Class from string in numerous places. Would it make sense to do this work once and store the value for later use?

> Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm:46
> +        return;

We check this selector in many places. I wonder if we could have a static or never-destroyed Class where we check this stuff once for the run of the application?

> Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm:83
> +    auto context = [NSClassFromString(@"LSDatabaseContext") sharedDatabaseContext];

This is the third time we've done this. Could this be stored?

> Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:48
> +                // Uncomment before landing; this is commented out because the bots does not seem to rebuild the entitlements

I don't understand this -- does this mean it will start failing as soon as you land the change? Or is this just an EWS issue?

> Source/WebKit/Shared/Cocoa/XPCEndpointClient.mm:64
> +                // return;

Ditto.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #20)
> Comment on attachment 403614 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403614&action=review
> 
> I think this looks fine, but I had a few questions. I also think it would be
> good for Jiewen or someone else with XPC experience to take a look at the
> implementation.
> 
> > Source/WebKit/ChangeLog:9
> > +        sent from the Networking process to the WebContent process, represented by a xpc object. The Networking process is chosen instead of the UI
> 
> "represented by AN xpc object." (and elsewhere in the summary).
> 
> > Source/WebKit/ChangeLog:22
> > +        No new tests, since there already exist tests for this. UTI to MIME type mapping is being used in WebKit and underlying framework, and many
> 
> underlying frameworks, ...
> 
> > Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm:44
> > +    auto context = [NSClassFromString(@"LSDatabaseContext") sharedDatabaseContext];
> 
> We seem to construct this Class from string in numerous places. Would it
> make sense to do this work once and store the value for later use?
> 
> > Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm:46
> > +        return;
> 
> We check this selector in many places. I wonder if we could have a static or
> never-destroyed Class where we check this stuff once for the run of the
> application?
> 
> > Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm:83
> > +    auto context = [NSClassFromString(@"LSDatabaseContext") sharedDatabaseContext];
> 
> This is the third time we've done this. Could this be stored?
> 

That is a good point. I will look into storing this.

> > Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:48
> > +                // Uncomment before landing; this is commented out because the bots does not seem to rebuild the entitlements
> 
> I don't understand this -- does this mean it will start failing as soon as
> you land the change? Or is this just an EWS issue?
> 

It seems the entitlements are not updated in an incremental build, so after landing we will have to start a clean build on the bots.

> > Source/WebKit/Shared/Cocoa/XPCEndpointClient.mm:64
> > +                // return;
> 
> Ditto.

Thanks for reviewing!

[Jiewen Tan]

Comment on attachment 403614 [details]
Patch

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

Thanks for making an effort to normalize XPC connections among different WebKit processes. The patch is a bit hard to review. I think the patch actually can be divided into two parts. It contains an effort to make a generic interface to do XPC after process bootstrap. And use that mechanism to do XPC for launch service database. A safer approach will be first make a patch about the generic interface and have it unit tested. Optionally, you could modify a well established XPC related functionality to adopt this generic interface. Then make the launch service database.

> Source/WebKit/ChangeLog:14
> +        each WebContent process with this endpoint by sending it over the bootstrap xpc connection between the UI process and the WebContent process.

Is it possible to not do the roundtrip to the UI Process? If it is not possible, I think we should explain why here.

> Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:56
> +            xpc_connection_resume(message);

I guess it should be:
    xpc_connection_resume(connection);

> Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:74
> +XPCEndpoint::~XPCEndpoint()

XPCEndpoint::~XPCEndpoint() = default;

You could do it in the header.

> Source/WebKit/Shared/Cocoa/XPCEndpointClient.mm:36
> +        LockHolder locker(m_lock);

Why do we need a lock here? I don't see any variables that can be shared among different threads.

> Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:65
> +        virtual RefPtr<XPCEventHandler> xpcEventHandler() const { return nullptr; }

I'm a little bit confused about the design here. Maybe you could illustrate it a bit more on the change log. I think you want to maintain a 1 to many mapping here. Can we have a similar design as the WebProcessPool such that we don't lose track of the owner/lifetime of the event handlers?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:342
> +        virtual ~XPCEventHandler();

Why do we need the destructor?

[Per Arne Vollan]

Created attachment 403721 [details]
Patch

[Per Arne Vollan]

(In reply to Jiewen Tan from comment #22)
> Comment on attachment 403614 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403614&action=review
> 
> Thanks for making an effort to normalize XPC connections among different
> WebKit processes. The patch is a bit hard to review. I think the patch
> actually can be divided into two parts. It contains an effort to make a
> generic interface to do XPC after process bootstrap. And use that mechanism
> to do XPC for launch service database. A safer approach will be first make a
> patch about the generic interface and have it unit tested. Optionally, you
> could modify a well established XPC related functionality to adopt this
> generic interface. Then make the launch service database.
> 

That is a very good point. In the latest patch, I added a unit test of the generic interfaces. I did not split this into two patches because of the extra overhead, but I moved the generic interfaces to WebCore to make the distinction clearer and allow them to be unit-tested. Are you OK with this approach, Jiewen?

> > Source/WebKit/ChangeLog:14
> > +        each WebContent process with this endpoint by sending it over the bootstrap xpc connection between the UI process and the WebContent process.
> 
> Is it possible to not do the roundtrip to the UI Process? If it is not
> possible, I think we should explain why here.
> 

No, this is not possible, because the direct connection between the Networking process and the WebContent process is a lower level CoreIPC connection, which cannot be used to send xpc objects. I added an explanation of this in the change log.

> > Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:56
> > +            xpc_connection_resume(message);
> 
> I guess it should be:
>     xpc_connection_resume(connection);
> 

Fixed.

> > Source/WebKit/Shared/Cocoa/XPCEndpoint.mm:74
> > +XPCEndpoint::~XPCEndpoint()
> 
> XPCEndpoint::~XPCEndpoint() = default;
> 
> You could do it in the header.
> 

Done!

> > Source/WebKit/Shared/Cocoa/XPCEndpointClient.mm:36
> > +        LockHolder locker(m_lock);
> 
> Why do we need a lock here? I don't see any variables that can be shared
> among different threads.
> 

The m_connections member can be set to nullptr on a non-main thread.

> > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:65
> > +        virtual RefPtr<XPCEventHandler> xpcEventHandler() const { return nullptr; }
> 
> I'm a little bit confused about the design here. Maybe you could illustrate
> it a bit more on the change log. I think you want to maintain a 1 to many
> mapping here. Can we have a similar design as the WebProcessPool such that
> we don't lose track of the owner/lifetime of the event handlers?
> 

Good point, I added a paragraph in the change log to describe this more detailed. The event handler on the bootstrap xpc connection is capturing this RefPtr to the XPCEventHandler, making sure that it is not destroyed while the event handler is active. The XPCEventHandler will in this case have a weak pointer to the NetworkProcessProxy, making sure we don't call into a deleted proxy.

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:342
> > +        virtual ~XPCEventHandler();
> 
> Why do we need the destructor?

Removed.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 403731 [details]
Patch

[Per Arne Vollan]

Created attachment 403733 [details]
Patch

[Brent Fulgham]

Comment on attachment 403733 [details]
Patch

This patch appears to include the changes in Bug 214079.

[Per Arne Vollan]

Created attachment 403804 [details]
Patch

[Brent Fulgham]

Comment on attachment 403804 [details]
Patch

LGTM. r+

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #29)
> Comment on attachment 403804 [details]
> Patch
> 
> LGTM. r+

Thanks for reviewing!

[EWS]

Committed r264132: <https://trac.webkit.org/changeset/264132>

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

(In reply to EWS from comment #31)
> Committed r264132: <https://trac.webkit.org/changeset/264132>

This seems to have broken building against the Apple internal SDK, because LSDatabaseContext is being redeclared outside of an #if !USE(APPLE_INTERNAL_SDK) block.

[Per Arne Vollan]

(In reply to mitz from comment #32)
> (In reply to EWS from comment #31)
> > Committed r264132: <https://trac.webkit.org/changeset/264132>
> 
> This seems to have broken building against the Apple internal SDK, because
> LSDatabaseContext is being redeclared outside of an #if
> !USE(APPLE_INTERNAL_SDK) block.

Ah, will fix, thanks!

[Yusuke Suzuki]

Re-opened since this is blocked by bug 214108

[Per Arne Vollan]

Created attachment 403836 [details]
Patch

[EWS]

Committed r264148: <https://trac.webkit.org/changeset/264148>

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

[Yusuke Suzuki]

Re-opened since this is blocked by bug 214136

[Per Arne Vollan]

Created attachment 403862 [details]
Patch

[EWS]

Committed r264178: <https://trac.webkit.org/changeset/264178>

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