Bug 215460 – [Cocoa] Avoid changing XPC target queue inside XPC event handler

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

In WebProcess::handleXPCEndpointMessages we currently change the XPC target queue for the XPC bootstrap connection while under the XPC event handler. This sometimes causes simulated crashes on iOS and should be avoided. According to the documentation in https://developer.apple.com/documentation/xpc/1448786-xpc_connection_set_target_queue?language=objc, there does not seem to be anything saying this is a programming error, but the simulated crash claims it is.

[Per Arne Vollan]

Created attachment 406534 [details]
Patch

[Per Arne Vollan]

Created attachment 406536 [details]
Patch

[Per Arne Vollan]

Created attachment 406538 [details]
Patch

[Per Arne Vollan]

Created attachment 406550 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/67025658>

[Brent Fulgham]

Comment on attachment 406550 [details]
Patch

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

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=215460

<rdar://problem/67025658>

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1571
> +    bool databaseUpdated = LaunchServicesDatabaseManager::singleton().waitForDatabaseUpdate(0_s);

Is there any way to register for a notification from LaunchServices, so we could react to a message rather than polling with a timer?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1577
> +        Vector<LoadParameters> loads = WTFMove(*m_pendingLoads);

Perhaps auto?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1593
> +            m_pendingLoadsTimer.startRepeating(100_ms);

If we only use this when the database update has not completed yet, could these loads be fired by the database update handler, rather than creating a new timer?

[Brent Fulgham]

David actually noticed this earlier:

<rdar://problem/65776813>

[Per Arne Vollan]

Created attachment 406604 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #6)
> Comment on attachment 406550 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406550&action=review
> 
> > Source/WebKit/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=215460
> 
> <rdar://problem/67025658>
> 

Done.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1571
> > +    bool databaseUpdated = LaunchServicesDatabaseManager::singleton().waitForDatabaseUpdate(0_s);
> 
> Is there any way to register for a notification from LaunchServices, so we
> could react to a message rather than polling with a timer?
> 

I don't think we currently get a notification, but we do know when we get the database in the WebContent process. However, to keep the patch as simple as possible, I think it would be good to do this as a future improvement. Do you think that is OK?

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1577
> > +        Vector<LoadParameters> loads = WTFMove(*m_pendingLoads);
> 
> Perhaps auto?
> 

Fixed.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1593
> > +            m_pendingLoadsTimer.startRepeating(100_ms);
> 
> If we only use this when the database update has not completed yet, could
> these loads be fired by the database update handler, rather than creating a
> new timer?

Yes, I think it could, but perhaps it would add complexity to have the database update handler have knowledge about loads and Web pages? Perhaps the Web page could register for some sort of notification as you earlier suggested. Would you be OK with improving this in a follow-up patch?


Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 406604 [details]
Patch

I think this looks good, but I'd like Chris or Geoff to double-check the XPC-related bits.

[Brent Fulgham]

Chris or Geoff: Could you double-check the XPC related bits of this change?

[Chris Dumez]

Comment on attachment 406604 [details]
Patch

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

This change seems wrong. What if the client app starts a load (the load gets queued by your new logic) and then the client app cancels the load (calling WebPageProxy::stopLoading()). It won't cancel the load anymore after your change.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1589
> +        WTFLogAlways("Launch Services database not updated when load requested.");

I assume you did not mean to keep a WTFLogAlways() in your patch?

> Source/WebKit/WebProcess/WebPage/WebPage.h:2151
> +    Optional<Vector<LoadParameters>> m_pendingLoads;

Why do we need an Optional<Vector>? Cannot we simply use the fact that the vector is empty as a hint?

[Chris Dumez]

Comment on attachment 406604 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        [Cocoa] Avoid changing XPC target queue inside XPC event handler

The patch seems to be doing more than this.

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 406604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406604&action=review
> 
> This change seems wrong. What if the client app starts a load (the load gets
> queued by your new logic) and then the client app cancels the load (calling
> WebPageProxy::stopLoading()). It won't cancel the load anymore after your
> change.

Note that stopLoading() is only example. I am sure there are other things that get broken with this new queueing. Probably, the client doing loadRequest: and then reload: while the load is queued. Or GoToBackForwardItem while a loadRequest is queued. Or LoadData while a loadRequest is queued.. To name a few.

> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1589
> > +        WTFLogAlways("Launch Services database not updated when load requested.");
> 
> I assume you did not mean to keep a WTFLogAlways() in your patch?
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:2151
> > +    Optional<Vector<LoadParameters>> m_pendingLoads;
> 
> Why do we need an Optional<Vector>? Cannot we simply use the fact that the
> vector is empty as a hint?

[Chris Dumez]

Comment on attachment 406604 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1586
> +#if HAVE(LSDATABASECONTEXT)

I will also note that moving the code from platformDidReceiveLoadParameters() to WebPage::loadRequest() has a pretty significant behavior difference. It means for example that it no longer impacts WebPage::loadData() which is another way for clients to do loads. Why is it OK to load a HTML String with a stale LaunchServices DB but not OK to load a URL with a stale database? What's the difference? Note that there is no comment in the code explaining why we need to update this database in the first place.

[Per Arne Vollan]

(In reply to Chris Dumez from comment #14)
> (In reply to Chris Dumez from comment #12)
> > Comment on attachment 406604 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=406604&action=review
> > 
> > This change seems wrong. What if the client app starts a load (the load gets
> > queued by your new logic) and then the client app cancels the load (calling
> > WebPageProxy::stopLoading()). It won't cancel the load anymore after your
> > change.
> 
> Note that stopLoading() is only example. I am sure there are other things
> that get broken with this new queueing. Probably, the client doing
> loadRequest: and then reload: while the load is queued. Or
> GoToBackForwardItem while a loadRequest is queued. Or LoadData while a
> loadRequest is queued.. To name a few.
> 

That is a good catch! I will look closer into resolving this.

Thanks for reviewing!

> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1589
> > > +        WTFLogAlways("Launch Services database not updated when load requested.");
> > 
> > I assume you did not mean to keep a WTFLogAlways() in your patch?
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.h:2151
> > > +    Optional<Vector<LoadParameters>> m_pendingLoads;
> > 
> > Why do we need an Optional<Vector>? Cannot we simply use the fact that the
> > vector is empty as a hint?

[Per Arne Vollan]

(In reply to Chris Dumez from comment #15)
> Comment on attachment 406604 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406604&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1586
> > +#if HAVE(LSDATABASECONTEXT)
> 
> I will also note that moving the code from
> platformDidReceiveLoadParameters() to WebPage::loadRequest() has a pretty
> significant behavior difference. It means for example that it no longer
> impacts WebPage::loadData() which is another way for clients to do loads.
> Why is it OK to load a HTML String with a stale LaunchServices DB but not OK
> to load a URL with a stale database? What's the difference? Note that there
> is no comment in the code explaining why we need to update this database in
> the first place.

You're absolutely right. The Launch services database is needed in both cases. The reason the database is needed is that it contains MIME type mapping information we use.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 406618 [details]
Patch

[Darin Adler]

Comment on attachment 406618 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:91
> +                dispatch_sync(dispatch_get_main_queue(), [initializerFunctionPtr = initializerFunctionPtr, peer = OSObjectPtr(peer), event = OSObjectPtr(event), priorityBoostMessage = OSObjectPtr(priorityBoostMessage)] {

The capturing isn’t right here. It’s overzealous.

"initializerFunctionPtr = initializerFunctionPtr" is not different/better than "initializerFunctionPtr". Given this is dispatch_sync I don’t think you need to use OSObjectPtr; this thread is holding on to retain counts and is blocked until dispatch_sync returns.

You could use simpler capturing.

> Source/WebKit/WebProcess/cocoa/HandleXPCEndpointMessages.mm:46
> +#if HAVE(LSDATABASECONTEXT)

Seems like this #if should be around more of the function body. Why wrap it so tight?

[Per Arne Vollan]

(In reply to Darin Adler from comment #19)
> Comment on attachment 406618 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406618&action=review
> 
> > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:91
> > +                dispatch_sync(dispatch_get_main_queue(), [initializerFunctionPtr = initializerFunctionPtr, peer = OSObjectPtr(peer), event = OSObjectPtr(event), priorityBoostMessage = OSObjectPtr(priorityBoostMessage)] {
> 
> The capturing isn’t right here. It’s overzealous.
> 
> "initializerFunctionPtr = initializerFunctionPtr" is not different/better
> than "initializerFunctionPtr". Given this is dispatch_sync I don’t think you
> need to use OSObjectPtr; this thread is holding on to retain counts and is
> blocked until dispatch_sync returns.
> 
> You could use simpler capturing.
> 

Good point! Will fix.

> > Source/WebKit/WebProcess/cocoa/HandleXPCEndpointMessages.mm:46
> > +#if HAVE(LSDATABASECONTEXT)
> 
> Seems like this #if should be around more of the function body. Why wrap it
> so tight?

This file is intended to handle other types of new XPC messages sent from the UI process, while we currently only support the Launch Services XPC endpoint message.

Thanks for reviewing!

[Darin Adler]

Comment on attachment 406618 [details]
Patch

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

>>> Source/WebKit/WebProcess/cocoa/HandleXPCEndpointMessages.mm:46
>>> +#if HAVE(LSDATABASECONTEXT)
>> 
>> Seems like this #if should be around more of the function body. Why wrap it so tight?
> 
> This file is intended to handle other types of new XPC messages sent from the UI process, while we currently only support the Launch Services XPC endpoint message.
> 
> Thanks for reviewing!

Yes, and we can move the #if when we add more message types.

[Per Arne Vollan]

(In reply to Darin Adler from comment #21)
> Comment on attachment 406618 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406618&action=review
> 
> >>> Source/WebKit/WebProcess/cocoa/HandleXPCEndpointMessages.mm:46
> >>> +#if HAVE(LSDATABASECONTEXT)
> >> 
> >> Seems like this #if should be around more of the function body. Why wrap it so tight?
> > 
> > This file is intended to handle other types of new XPC messages sent from the UI process, while we currently only support the Launch Services XPC endpoint message.
> > 
> > Thanks for reviewing!
> 
> Yes, and we can move the #if when we add more message types.

Ah, I see what you mean. Will fix.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 406629 [details]
Patch

[EWS]

Committed r265715: <https://trac.webkit.org/changeset/265715>

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

[Jonathan Bedard]

iOS API tests are crashing due to this commit:
<https://results.webkit.org/suites?suite=api-tests&platform=ios>