Bug 213794 - [Cocoa] Update Launch Services database in the WebContent process from the Network process
Summary: [Cocoa] Update Launch Services database in the WebContent process from the Ne...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on: 214108 214136
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-30 07:08 PDT by Per Arne Vollan
Modified: 2020-07-10 14:50 PDT (History)
14 users (show)

See Also:


Attachments
Patch (32.68 KB, patch)
2020-06-30 08:04 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.28 KB, patch)
2020-06-30 10:04 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.40 KB, patch)
2020-06-30 11:21 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.06 KB, patch)
2020-06-30 11:38 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (36.98 KB, patch)
2020-06-30 11:47 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.29 KB, patch)
2020-06-30 12:11 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.19 KB, patch)
2020-06-30 12:39 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.25 KB, patch)
2020-06-30 13:11 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (44.73 KB, patch)
2020-07-02 14:35 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (44.93 KB, patch)
2020-07-02 14:46 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (47.27 KB, patch)
2020-07-02 15:12 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (47.65 KB, patch)
2020-07-02 16:06 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (62.21 KB, patch)
2020-07-06 06:39 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (63.20 KB, patch)
2020-07-06 07:25 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (68.35 KB, patch)
2020-07-06 07:35 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (67.41 KB, patch)
2020-07-06 07:59 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (70.56 KB, patch)
2020-07-06 11:55 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (70.59 KB, patch)
2020-07-06 13:22 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (84.40 KB, patch)
2020-07-07 14:00 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (85.39 KB, patch)
2020-07-07 14:53 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (85.42 KB, patch)
2020-07-07 15:32 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (56.06 KB, patch)
2020-07-08 13:21 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (57.79 KB, patch)
2020-07-08 19:30 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (58.94 KB, patch)
2020-07-09 07:15 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2020-06-30 07:08:42 PDT
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.
Comment 1 Per Arne Vollan 2020-06-30 08:04:06 PDT
Created attachment 403200 [details]
Patch
Comment 2 Per Arne Vollan 2020-06-30 10:04:08 PDT
Created attachment 403205 [details]
Patch
Comment 3 Per Arne Vollan 2020-06-30 11:21:00 PDT
Created attachment 403210 [details]
Patch
Comment 4 Per Arne Vollan 2020-06-30 11:38:09 PDT
Created attachment 403211 [details]
Patch
Comment 5 Per Arne Vollan 2020-06-30 11:47:57 PDT
Created attachment 403214 [details]
Patch
Comment 6 Per Arne Vollan 2020-06-30 12:11:20 PDT
Created attachment 403220 [details]
Patch
Comment 7 Per Arne Vollan 2020-06-30 12:39:55 PDT
Created attachment 403227 [details]
Patch
Comment 8 Per Arne Vollan 2020-06-30 13:11:01 PDT
Created attachment 403232 [details]
Patch
Comment 9 Brent Fulgham 2020-06-30 13:26:00 PDT
<rdar://problem/64168895>
Comment 10 Per Arne Vollan 2020-07-02 14:35:00 PDT
Created attachment 403397 [details]
Patch
Comment 11 Per Arne Vollan 2020-07-02 14:46:26 PDT
Created attachment 403399 [details]
Patch
Comment 12 Per Arne Vollan 2020-07-02 15:12:21 PDT
Created attachment 403405 [details]
Patch
Comment 13 Per Arne Vollan 2020-07-02 16:06:08 PDT
Created attachment 403414 [details]
Patch
Comment 14 Per Arne Vollan 2020-07-06 06:39:21 PDT
Created attachment 403589 [details]
Patch
Comment 15 Per Arne Vollan 2020-07-06 07:25:37 PDT
Created attachment 403592 [details]
Patch
Comment 16 Per Arne Vollan 2020-07-06 07:35:25 PDT
Created attachment 403593 [details]
Patch
Comment 17 Per Arne Vollan 2020-07-06 07:59:15 PDT
Created attachment 403595 [details]
Patch
Comment 18 Per Arne Vollan 2020-07-06 11:55:50 PDT
Created attachment 403603 [details]
Patch
Comment 19 Per Arne Vollan 2020-07-06 13:22:39 PDT
Created attachment 403614 [details]
Patch
Comment 20 Brent Fulgham 2020-07-06 16:02:19 PDT
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.
Comment 21 Per Arne Vollan 2020-07-06 16:23:08 PDT
(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!
Comment 22 Jiewen Tan 2020-07-06 22:03:51 PDT
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?
Comment 23 Per Arne Vollan 2020-07-07 14:00:33 PDT
Created attachment 403721 [details]
Patch
Comment 24 Per Arne Vollan 2020-07-07 14:14:22 PDT
(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!
Comment 25 Per Arne Vollan 2020-07-07 14:53:11 PDT
Created attachment 403731 [details]
Patch
Comment 26 Per Arne Vollan 2020-07-07 15:32:05 PDT
Created attachment 403733 [details]
Patch
Comment 27 Brent Fulgham 2020-07-08 11:24:38 PDT
Comment on attachment 403733 [details]
Patch

This patch appears to include the changes in Bug 214079.
Comment 28 Per Arne Vollan 2020-07-08 13:21:23 PDT
Created attachment 403804 [details]
Patch
Comment 29 Brent Fulgham 2020-07-08 14:05:04 PDT
Comment on attachment 403804 [details]
Patch

LGTM. r+
Comment 30 Per Arne Vollan 2020-07-08 14:07:22 PDT
(In reply to Brent Fulgham from comment #29)
> Comment on attachment 403804 [details]
> Patch
> 
> LGTM. r+

Thanks for reviewing!
Comment 31 EWS 2020-07-08 14:13:59 PDT
Committed r264132: <https://trac.webkit.org/changeset/264132>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403804 [details].
Comment 32 mitz 2020-07-08 15:28:17 PDT
(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.
Comment 33 Per Arne Vollan 2020-07-08 15:30:19 PDT
(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!
Comment 34 Yusuke Suzuki 2020-07-08 16:45:20 PDT
Re-opened since this is blocked by bug 214108
Comment 35 Per Arne Vollan 2020-07-08 19:30:23 PDT
Created attachment 403836 [details]
Patch
Comment 36 EWS 2020-07-08 22:24:41 PDT
Committed r264148: <https://trac.webkit.org/changeset/264148>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403836 [details].
Comment 37 Yusuke Suzuki 2020-07-09 06:59:02 PDT
Re-opened since this is blocked by bug 214136
Comment 38 Per Arne Vollan 2020-07-09 07:15:38 PDT
Created attachment 403862 [details]
Patch
Comment 39 EWS 2020-07-09 10:06:19 PDT
Committed r264178: <https://trac.webkit.org/changeset/264178>

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