Summary: | [Cocoa] Update Launch Services database in the WebContent process from the Network process | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, andersca, benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, ggaren, jiewen_tan, mitz, sam, simon.fraser, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=214200 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 214108, 214136 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2020-06-30 07:08:42 PDT
Created attachment 403200 [details]
Patch
Created attachment 403205 [details]
Patch
Created attachment 403210 [details]
Patch
Created attachment 403211 [details]
Patch
Created attachment 403214 [details]
Patch
Created attachment 403220 [details]
Patch
Created attachment 403227 [details]
Patch
Created attachment 403232 [details]
Patch
Created attachment 403397 [details]
Patch
Created attachment 403399 [details]
Patch
Created attachment 403405 [details]
Patch
Created attachment 403414 [details]
Patch
Created attachment 403589 [details]
Patch
Created attachment 403592 [details]
Patch
Created attachment 403593 [details]
Patch
Created attachment 403595 [details]
Patch
Created attachment 403603 [details]
Patch
Created attachment 403614 [details]
Patch
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. (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 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? Created attachment 403721 [details]
Patch
(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! Created attachment 403731 [details]
Patch
Created attachment 403733 [details]
Patch
Comment on attachment 403733 [details] Patch This patch appears to include the changes in Bug 214079. Created attachment 403804 [details]
Patch
Comment on attachment 403804 [details]
Patch
LGTM. r+
(In reply to Brent Fulgham from comment #29) > Comment on attachment 403804 [details] > Patch > > LGTM. r+ Thanks for reviewing! 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. (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! Re-opened since this is blocked by bug 214108 Created attachment 403836 [details]
Patch
Committed r264148: <https://trac.webkit.org/changeset/264148> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403836 [details]. Re-opened since this is blocked by bug 214136 Created attachment 403862 [details]
Patch
Committed r264178: <https://trac.webkit.org/changeset/264178> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403862 [details]. |