Bug 215460

Created attachment 406534 [details]
Patch

Created attachment 406536 [details]
Patch

Created attachment 406538 [details]
Patch

Created attachment 406550 [details]
Patch

<rdar://problem/67025658>

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?

David actually noticed this earlier:

<rdar://problem/65776813>

Created attachment 406604 [details]
Patch

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

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.

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

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?

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.

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

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.

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

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

Created attachment 406618 [details]
Patch

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?

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

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.

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

Created attachment 406629 [details]
Patch

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

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

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