Bug 235186

Summary: Inject Launch Services database before NSApplication is initialized
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, simon.fraser, thorton, w0nka, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 235341    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
cdumez: review+
Patch
none
Patch none

Description Per Arne Vollan 2022-01-13 09:00:29 PST 
To avoid the main thread getting stuck, the Launch Services database should be injected before NSApplication is initialized, since the initialization now depends on the database.
Comment 1 Per Arne Vollan 2022-01-13 09:01:37 PST 
<rdar://87468788>
Comment 2 Per Arne Vollan 2022-01-13 09:09:04 PST 
Created attachment 449067 [details]
Patch
Comment 3 Chris Dumez 2022-01-13 09:14:09 PST 
Comment on attachment 449067 [details]
Patch

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

r=me

> Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:87
> +    auto startTime = WallTime::now();

Should probably be using MonotonicTime::now()

> Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:89
> +    auto elapsedTime = WallTime::now() - startTime;

ditto.

> Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:91
> +        RELEASE_LOG(Loading, "Waiting for Launch Services database update took %f seconds", elapsedTime.value());

Can we use RELEASE_LOG_ERROR so that it stands out in the logs? Any delay during process initialization is very visible to the user and should be avoided.
Comment 4 Per Arne Vollan 2022-01-13 09:29:35 PST 
Created attachment 449072 [details]
Patch
Comment 5 Per Arne Vollan 2022-01-13 09:30:09 PST 
(In reply to Chris Dumez from comment #3)
> Comment on attachment 449067 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449067&action=review
> 
> r=me
> 
> > Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:87
> > +    auto startTime = WallTime::now();
> 
> Should probably be using MonotonicTime::now()
> 
> > Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:89
> > +    auto elapsedTime = WallTime::now() - startTime;
> 
> ditto.
> 
> > Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:91
> > +        RELEASE_LOG(Loading, "Waiting for Launch Services database update took %f seconds", elapsedTime.value());
> 
> Can we use RELEASE_LOG_ERROR so that it stands out in the logs? Any delay
> during process initialization is very visible to the user and should be
> avoided.

Done!

Thanks for reviewing!
Comment 6 Simon Fraser (smfr) 2022-01-13 09:53:59 PST 
Comment on attachment 449072 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:90
> +    if (elapsedTime.value() > 0.5)

if (elapsedTime > 0.5_s)
Comment 7 Chris Dumez 2022-01-13 09:54:32 PST 
(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 449072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449072&action=review
> 
> > Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:90
> > +    if (elapsedTime.value() > 0.5)
> 
> if (elapsedTime > 0.5_s)

Nicer indeed
Comment 8 Per Arne Vollan 2022-01-13 10:02:38 PST 
(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 449072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449072&action=review
> 
> > Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:90
> > +    if (elapsedTime.value() > 0.5)
> 
> if (elapsedTime > 0.5_s)

Will fix!

Thanks for reviewing!
Comment 9 Per Arne Vollan 2022-01-13 10:19:56 PST 
Created attachment 449079 [details]
Patch
Comment 10 Per Arne Vollan 2022-01-13 14:18:54 PST 
Created attachment 449110 [details]
Patch
Comment 11 Per Arne Vollan 2022-01-14 07:24:01 PST 
Created attachment 449171 [details]
Patch
Comment 12 EWS 2022-01-14 09:11:55 PST 
Committed r288019 (246045@main): <https://commits.webkit.org/246045@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449171 [details].
Comment 14 Per Arne Vollan 2022-01-17 17:21:22 PST 
Created attachment 449359 [details]
Patch
Comment 15 Per Arne Vollan 2022-01-17 17:22:46 PST 
Reopened, due to debug assert on EWS.
Comment 16 Per Arne Vollan 2022-01-18 07:20:34 PST 
Created attachment 449386 [details]
Patch
Comment 17 Simon Fraser (smfr) 2022-01-18 08:36:35 PST 
Comment on attachment 449386 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:93
> +        fprintf(stderr, "Waiting for Launch Services database update took %f seconds", elapsedTime.value());

WTFLogAlways.
Comment 18 Wenson Hsieh 2022-01-18 10:51:27 PST 
Comment on attachment 449386 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:36
> +#import <wtf/NumberOfCores.h>

Nit - do we need to import this header?
Comment 19 Per Arne Vollan 2022-01-18 11:27:25 PST 
(In reply to Simon Fraser (smfr) from comment #17)
> Comment on attachment 449386 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449386&action=review
> 
> > Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:93
> > +        fprintf(stderr, "Waiting for Launch Services database update took %f seconds", elapsedTime.value());
> 
> WTFLogAlways.

This patch is for debugging purposes, currently. I am speculating that writing to stderr will give us information about which test is sometimes hitting the debug assert.

Thanks for reviewing!
Comment 20 Per Arne Vollan 2022-01-18 16:22:39 PST 
(In reply to Wenson Hsieh from comment #18)
> Comment on attachment 449386 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449386&action=review
> 
> > Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm:36
> > +#import <wtf/NumberOfCores.h>
> 
> Nit - do we need to import this header?

Good catch! So far this patch is for debugging purposes, but will remove when a new patch is ready for review.

Thanks for reviewing!
Comment 21 Per Arne Vollan 2022-01-19 13:14:20 PST 
Created attachment 449510 [details]
Patch
Comment 22 Chris Dumez 2022-01-19 13:30:51 PST 
Comment on attachment 449510 [details]
Patch

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

This patch fails to update GPUProcessProxy.cpp accordingly. We can simplify things there now that sendNetworkProcessXPCEndpointToProcess() no longer needs a data store instance.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:281
> +    static bool sendNetworkProcessXPCEndpointToProcess(AuxiliaryProcessProxy&);

Now that it is static and doesn't use any information from the data store, I would suggest moving this to NetworkProcessProxy and make it non-static. Then WebProcessProxy could call `NetworkProcessProxy::ensureDefaultNetworkProcess().sendXPCEndpointToProcess(*this)`
Comment 23 Per Arne Vollan 2022-01-20 10:06:11 PST 
Created attachment 449593 [details]
Patch
Comment 24 Per Arne Vollan 2022-01-20 10:11:07 PST 
(In reply to Chris Dumez from comment #22)
> Comment on attachment 449510 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449510&action=review
> 
> This patch fails to update GPUProcessProxy.cpp accordingly. We can simplify
> things there now that sendNetworkProcessXPCEndpointToProcess() no longer
> needs a data store instance.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:281
> > +    static bool sendNetworkProcessXPCEndpointToProcess(AuxiliaryProcessProxy&);
> 
> Now that it is static and doesn't use any information from the data store, I
> would suggest moving this to NetworkProcessProxy and make it non-static.
> Then WebProcessProxy could call
> `NetworkProcessProxy::ensureDefaultNetworkProcess().
> sendXPCEndpointToProcess(*this)`

Fixed in latest patch.

Thanks for reviewing!
Comment 25 Chris Dumez 2022-01-20 10:20:26 PST 
Comment on attachment 449593 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:274
> +    bool sendNetworkProcessXPCEndpointToProcess(AuxiliaryProcessProxy&);

Personally, I feel we could drop "NetworkProcess" from the name now that it is located on the NetworkProcessProxy class. Your call.

> Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm:61
> +        if (GPUProcessProxy::singletonIfCreated())

if (auto gpuProcess = GPUProcessProxy::singletonIfCreated())

> Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm:62
> +            m_networkProcess->sendNetworkProcessXPCEndpointToProcess(*GPUProcessProxy::singletonIfCreated());

*gpuProcess
Comment 26 Per Arne Vollan 2022-01-20 15:59:31 PST 
Created attachment 449615 [details]
Patch
Comment 27 Per Arne Vollan 2022-01-20 16:00:33 PST 
(In reply to Chris Dumez from comment #25)
> Comment on attachment 449593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449593&action=review
> 
> r=me
> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:274
> > +    bool sendNetworkProcessXPCEndpointToProcess(AuxiliaryProcessProxy&);
> 
> Personally, I feel we could drop "NetworkProcess" from the name now that it
> is located on the NetworkProcessProxy class. Your call.
> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm:61
> > +        if (GPUProcessProxy::singletonIfCreated())
> 
> if (auto gpuProcess = GPUProcessProxy::singletonIfCreated())
> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm:62
> > +            m_networkProcess->sendNetworkProcessXPCEndpointToProcess(*GPUProcessProxy::singletonIfCreated());
> 
> *gpuProcess

Fixed!

Thanks for reviewing!
Comment 28 Per Arne Vollan 2022-01-21 11:30:31 PST 
Created attachment 449674 [details]
Patch
Comment 29 EWS 2022-01-21 16:30:19 PST 
Committed r288387 (246284@main): <https://commits.webkit.org/246284@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449674 [details].
Comment 30 Per Arne Vollan 2022-01-27 13:02:05 PST 
*** Bug 235719 has been marked as a duplicate of this bug. ***