Bug 198050 – Handling for non-persistent data should be consistent in computeNetworkProcessAccessTypeForDataFetch

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

... as title.

[Sihui Liu]

Created attachment 370465 [details]
Patch

[youenn fablet]

Comment on attachment 370465 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:223
> +            break;

Nice to use ownerProcess.

There is probably no need for std::max.
We could write it as:
bool hasNetworkDataType =  WTF::anyOf(dataTypes, [](auto& type) { return type == WebsiteDataProcessType::Network; });
if (!hasNetworkType)
   return ProcessAccessType::None;
return isNonPersistentStore ? ... 

Can we add an API test as well?

[Sihui Liu]

(In reply to youenn fablet from comment #2)
> Comment on attachment 370465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370465&action=review
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:223
> > +            break;
> 
> Nice to use ownerProcess.
> 
> There is probably no need for std::max.
> We could write it as:
> bool hasNetworkDataType =  WTF::anyOf(dataTypes, [](auto& type) { return
> type == WebsiteDataProcessType::Network; });
> if (!hasNetworkType)
>    return ProcessAccessType::None;
> return isNonPersistentStore ? ... 
> 

I tried this before uploading the patch, anyOf doesn't work when iterating OptionSet as it uses lvalue reference.

> Can we add an API test as well?

It turns out websiteDataStore will create temporary processPool for the datastore operations. The processpool is gone when operation finishes, so we cannot check whether it has network process in the operation callback.

Any other idea?

[Sihui Liu]

Created attachment 370514 [details]
Patch

[youenn fablet]

> I tried this before uploading the patch, anyOf doesn't work when iterating
> OptionSet as it uses lvalue reference.

Can you simplify the patch by not using std::max?

[youenn fablet]

(In reply to Sihui Liu from comment #4)
> Created attachment 370514 [details]
> Patch

Already done... r=me.

[youenn fablet]

As of the test, we should be able to remove non persistent data from the network process in memory structures. Then we can write such test.

[Sihui Liu]

(In reply to youenn fablet from comment #7)
> As of the test, we should be able to remove non persistent data from the
> network process in memory structures. Then we can write such test.

True, currently data of non-persistent datastore will not be fetched or removed from network process, because webprocesspool will not be associated with a non-persistent store and data is only fetched or removed from network process of associated processpool.

Probably we should either:
1. make websitedatastore tries to operate on all processpools, no matter they are associated or not
or
2. let processpool keep track of all websitedatastores in use

[WebKit Commit Bot]

Comment on attachment 370514 [details]
Patch

Clearing flags on attachment: 370514

Committed r245709: <https://trac.webkit.org/changeset/245709>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51080078>