Bug 215569 – [Cocoa] Avoid waiting for Launch Services on every load

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Currently, we are waiting for the Launch Services database to be present on every load in the WebContent process. It should be sufficient to wait only on the first load.

[Per Arne Vollan]

Created attachment 406715 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/67242794>

[Darin Adler]

Comment on attachment 406715 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:69
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {

This is a multi-thread-safe way of doing this. Can this be called from multiple threads? If not, a simple global check would suffice.

[Per Arne Vollan]

(In reply to Darin Adler from comment #3)
> Comment on attachment 406715 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406715&action=review
> 
> > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:69
> > +    static std::once_flag onceFlag;
> > +    std::call_once(onceFlag, [] {
> 
> This is a multi-thread-safe way of doing this. Can this be called from
> multiple threads? If not, a simple global check would suffice.

No, I don't believe this can be called from multiple threads. Will fix.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 406742 [details]
Patch

[Darin Adler]

Comment on attachment 406742 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:79
> +    };

This semicolon is not needed.

[Per Arne Vollan]

(In reply to Darin Adler from comment #6)
> Comment on attachment 406742 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406742&action=review
> 
> > Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:79
> > +    };
> 
> This semicolon is not needed.

Thanks, will fix :)

[Per Arne Vollan]

Created attachment 406743 [details]
Patch

[EWS]

Committed r265774: <https://trac.webkit.org/changeset/265774>

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

[Chris Dumez]

Comment on attachment 406743 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        increase the maximum wait time from 1s to 5s.

I don't think we are ever OK with hanging for 5 seconds. As a matter of fact, during this database update, the process is not processing incoming IPC anymore and gets reported as unresponsive to the client after 3 seconds. Clients such as mail kill the process right away which means that we hung but don't see the time out logging (because the process was terminated before reaching the timeout).

There are 2 issues here:
1. We should not be hanging in the first place
2. The 5 seconds timeout is too long. I'd suggest 1 or 2 seconds, something definitely below the 3 second timeout the UIProcess ResponsivenessTimer uses.

[Darin Adler]

Comment on attachment 406743 [details]
Patch

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

>> Source/WebKit/ChangeLog:10
>> +        increase the maximum wait time from 1s to 5s.
> 
> I don't think we are ever OK with hanging for 5 seconds. As a matter of fact, during this database update, the process is not processing incoming IPC anymore and gets reported as unresponsive to the client after 3 seconds. Clients such as mail kill the process right away which means that we hung but don't see the time out logging (because the process was terminated before reaching the timeout).
> 
> There are 2 issues here:
> 1. We should not be hanging in the first place
> 2. The 5 seconds timeout is too long. I'd suggest 1 or 2 seconds, something definitely below the 3 second timeout the UIProcess ResponsivenessTimer uses.

Change it back to the 1 second it was before this patch?

[Per Arne Vollan]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 406743 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406743&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +        increase the maximum wait time from 1s to 5s.
> 
> I don't think we are ever OK with hanging for 5 seconds. As a matter of
> fact, during this database update, the process is not processing incoming
> IPC anymore and gets reported as unresponsive to the client after 3 seconds.
> Clients such as mail kill the process right away which means that we hung
> but don't see the time out logging (because the process was terminated
> before reaching the timeout).
> 
> There are 2 issues here:
> 1. We should not be hanging in the first place

Yes. This seems to be caused by the entitlement check for "com.apple.private.webkit.use-xpc-endpoint" failing. I am currently looking into this.

> 2. The 5 seconds timeout is too long. I'd suggest 1 or 2 seconds, something
> definitely below the 3 second timeout the UIProcess ResponsivenessTimer uses.

That sounds good, I will change that.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Darin Adler from comment #11)
> Comment on attachment 406743 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406743&action=review
> 
> >> Source/WebKit/ChangeLog:10
> >> +        increase the maximum wait time from 1s to 5s.
> > 
> > I don't think we are ever OK with hanging for 5 seconds. As a matter of fact, during this database update, the process is not processing incoming IPC anymore and gets reported as unresponsive to the client after 3 seconds. Clients such as mail kill the process right away which means that we hung but don't see the time out logging (because the process was terminated before reaching the timeout).
> > 
> > There are 2 issues here:
> > 1. We should not be hanging in the first place
> > 2. The 5 seconds timeout is too long. I'd suggest 1 or 2 seconds, something definitely below the 3 second timeout the UIProcess ResponsivenessTimer uses.
> 
> Change it back to the 1 second it was before this patch?

Yes, I will change it back to 1 second.

Thanks for reviewing!