Bug 198050

Summary: Handling for non-persistent data should be consistent in computeNetworkProcessAccessTypeForDataFetch
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
youennf: review+
Patch none

Sihui Liu
Reported 2019-05-20 13:02:27 PDT
... as title.
Attachments
Patch (3.41 KB, patch)
2019-05-22 16:38 PDT, Sihui Liu 		youennf: review+
DetailsFormatted DiffDiff
Patch (3.52 KB, patch)
2019-05-23 11:50 PDT, Sihui Liu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sihui Liu
Comment 1 2019-05-22 16:38:36 PDT
Created attachment 370465 [details] Patch
youenn fablet
Comment 2 2019-05-23 08:55:42 PDT
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
Comment 3 2019-05-23 11:45:04 PDT
(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
Comment 4 2019-05-23 11:50:24 PDT
Created attachment 370514 [details] Patch
youenn fablet
Comment 5 2019-05-23 12:08:04 PDT
> 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
Comment 6 2019-05-23 12:08:48 PDT
(In reply to Sihui Liu from comment #4) > Created attachment 370514 [details] > Patch Already done... r=me.
youenn fablet
Comment 7 2019-05-23 12:09:27 PDT
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
Comment 8 2019-05-23 12:20:10 PDT
(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 9 2019-05-23 13:23:07 PDT
Comment on attachment 370514 [details] Patch Clearing flags on attachment: 370514 Committed r245709: <https://trac.webkit.org/changeset/245709>
WebKit Commit Bot
Comment 10 2019-05-23 13:23:09 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2019-05-23 13:24:30 PDT
<rdar://problem/51080078>
Note You need to log in before you can comment on or make changes to this bug.