Bug 232047 – Launch Services database is not always sent to GPUP

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Currently, the XPC endpoint for receiving the Launch Services database is being sent to GPUP in GPUProcessProxy::addSession. If GPUP has not finished launching at that point, the endpoint will not be sent.

[Per Arne Vollan]

Created attachment 441938 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84480229>

[Per Arne Vollan]

Created attachment 441943 [details]
Patch

[Brent Fulgham]

Comment on attachment 441943 [details]
Patch

r=me

[Per Arne Vollan]

Created attachment 441944 [details]
Patch

[Per Arne Vollan]

Created attachment 441945 [details]
Patch

[Per Arne Vollan]

Comment on attachment 441945 [details]
Patch

Thanks for reviewing!

[Darin Adler]

Comment on attachment 441938 [details]
Patch

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

Some additional thoughts.

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:439
> +    auto it = m_sessionIDs.begin();
> +    if (it != m_sessionIDs.end()) {
> +        auto webSiteDataStore = WebsiteDataStore::existingDataStoreForSessionID(*m_sessionIDs.begin());

This is a slightly unusual pattern. Since m_sessionIDs is a HashSet, choosing the first by iterating gives you an arbitrary member of the set, variable. If that is indeed what you want then I suppose this is OK, but note that this is neither truly random, nor fits any reliable pattern. Almost worth a comment explaining why it’s OK to use only one of the session IDs in a set, because I do not think that is obvious. Maybe just a comment saying "all session IDs are guaranteed to have the same data store".

Coding style wise, this calls begin() an extra time even though it uses a local variable. There are two ways I might write this that I think would be good style:

    // <Comment explaining why choosing an arbitrary session ID is OK, but random is not needed.>
    if (auto it = m_sessionIDs.begin(); it != m_sessionIDs.end()) {
        auto store = WebsiteDataStore::existingDataStoreForSessionID(*it);

Or:

    // <Comment explaining why choosing an arbitrary session ID is OK, but random is not needed.>
    if (!m_sessionIDs.isEmpty()) {
        auto store = WebsiteDataStore::existingDataStoreForSessionID(m_sessionIDs.begin());

Also, how solid is the guarantee that existingDataStoreForSessionID will not return null?

[Per Arne Vollan]

(In reply to Darin Adler from comment #8)
> Comment on attachment 441938 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441938&action=review
> 
> Some additional thoughts.
> 
> > Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:439
> > +    auto it = m_sessionIDs.begin();
> > +    if (it != m_sessionIDs.end()) {
> > +        auto webSiteDataStore = WebsiteDataStore::existingDataStoreForSessionID(*m_sessionIDs.begin());
> 
> This is a slightly unusual pattern. Since m_sessionIDs is a HashSet,
> choosing the first by iterating gives you an arbitrary member of the set,
> variable. If that is indeed what you want then I suppose this is OK, but
> note that this is neither truly random, nor fits any reliable pattern.
> Almost worth a comment explaining why it’s OK to use only one of the session
> IDs in a set, because I do not think that is obvious. Maybe just a comment
> saying "all session IDs are guaranteed to have the same data store".
> 
> Coding style wise, this calls begin() an extra time even though it uses a
> local variable. There are two ways I might write this that I think would be
> good style:
> 
>     // <Comment explaining why choosing an arbitrary session ID is OK, but
> random is not needed.>
>     if (auto it = m_sessionIDs.begin(); it != m_sessionIDs.end()) {
>         auto store = WebsiteDataStore::existingDataStoreForSessionID(*it);
> 
> Or:
> 
>     // <Comment explaining why choosing an arbitrary session ID is OK, but
> random is not needed.>
>     if (!m_sessionIDs.isEmpty()) {
>         auto store =
> WebsiteDataStore::existingDataStoreForSessionID(m_sessionIDs.begin());
> 

That is a good point. I will upload a revised patch, adopting one of these approaches and adding a comment. The reason any session ID is OK to use, is that we're looking to get the XPC endpoint from any Networking process. All Networking processes has this endpoint, and it does not matter from which Networking process we get it from.

> Also, how solid is the guarantee that existingDataStoreForSessionID will not
> return null?

That is also a good point. Although unlikely, I think it is possible that the data store for this session has been removed at this point, since this session ID is kept in a hash map not controlled by the data store class. I will add a null check here.

Thanks for reviewing!

[EWS]

Committed r284582 (243319@main): <https://commits.webkit.org/243319@main>

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

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 442032 [details]
Patch

[Per Arne Vollan]

Created attachment 442033 [details]
Patch

[Per Arne Vollan]

I believe the api-gtk are unrelated to the patch.

[EWS]

Committed r284641 (243361@main): <https://commits.webkit.org/243361@main>

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