Bug 235186 – Inject Launch Services database before NSApplication is initialized

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

[Per Arne Vollan]

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.

[Per Arne Vollan]

<rdar://87468788>

[Per Arne Vollan]

Created attachment 449067 [details]
Patch

[Chris Dumez]

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.

[Per Arne Vollan]

Created attachment 449072 [details]
Patch

[Per Arne Vollan]

(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!

[Simon Fraser (smfr)]

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)

[Chris Dumez]

(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

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 449079 [details]
Patch

[Per Arne Vollan]

Created attachment 449110 [details]
Patch

[Per Arne Vollan]

Created attachment 449171 [details]
Patch

[EWS]

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].

[Simon Fraser (smfr)]

EWS is hitting assertions added in this patch:
https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/r449267-21271/com.apple.WebKit.WebContent.Development-51883-crash-log.txt

[Per Arne Vollan]

Created attachment 449359 [details]
Patch

[Per Arne Vollan]

Reopened, due to debug assert on EWS.

[Per Arne Vollan]

Created attachment 449386 [details]
Patch

[Simon Fraser (smfr)]

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.

[Wenson Hsieh]

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?

[Per Arne Vollan]

(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!

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 449510 [details]
Patch

[Chris Dumez]

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)`

[Per Arne Vollan]

Created attachment 449593 [details]
Patch

[Per Arne Vollan]

(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!

[Chris Dumez]

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

[Per Arne Vollan]

Created attachment 449615 [details]
Patch

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 449674 [details]
Patch

[EWS]

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].

[Per Arne Vollan]

*** Bug 235719 has been marked as a duplicate of this bug. ***