[webkit-reviews] review denied: [Bug 202372] Updated resource load statistics are never merged into the SQLite Database backend : [Attachment 379866] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Katherine_cheney at apple.com's
request for review:
Bug 202372: Updated resource load statistics are never merged into the SQLite
Database backend
https://bugs.webkit.org/show_bug.cgi?id=202372

Attachment 379866: Patch

https://bugs.webkit.org/attachment.cgi?id=379866&action=review




--- Comment #5 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 379866
  --> https://bugs.webkit.org/attachment.cgi?id=379866
Patch

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

This is really close, but needs a bit of work before it's ready to land. I'm
glad to see all the tests pass!

Please change the %{public} to %{private} in the log statements

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:487
> +    for (auto& topFrameDomain :
loadStatistics.storageAccessUnderTopFrameDomains) {

We should probably get rid of these loops of SQLite queries and replace with a
single query that does all of the updates at once. To do so, you'll need to
revise the insert statements to ignore instances where the rows in the table
already exist.

Note: Let's do this improvement under a separate bug!

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:489
> +	      
insertDomainRelationship(m_storageAccessUnderTopFrameDomainsStatement,
registrableDomainID.value(), topFrameDomain);

Does attempting to insert an already-existing relationship cause an error? On
other SQL systems it used to be possible to indicate that you don't care if
something fails (because it already exists). That would remove the need to pay
the cost of this check when inserting.

This would involve changing the existing INSERT statements into "INSERT OR
IGNORE", which should be less closely than running a Query to see if a
relationship exists, then a second query to insert (if it doesn't exist).

>>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:547
>> +	return other.lastSeen <= mostRecentUserInteractionTime + 24_h;
> 
> Where do these 24 hours come from? I don't understand their meaning here. Is
something missing in the function name to explain the 24 hours?

This looks like some hold-over from the 24-hour window for user interaction
(which is no longer a thing). Kate: Check what this code does in the in-memory
store. We should match that behavior.

>>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:562
>> +	unsigned currentTimesAccessedDueToFirstPartyInteraction =
current.getColumnInt(9);
> 
> I'm wondering if these integers could be constants so that we don't mess it
up down the road, changing them in one place but forgetting about another.

I'm not sure how easy this would be, since it's controlled by the form of the
SQL statement and the columns in the database. Maybe you could define them at
the same point as the 'constexpr auto createObservedDomain = "CREATE TABLE...'
declarations, which would at least help keep them in sync. Ditto for the
parameters to the queries.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:571
> +	   // Else, do nothing.

I don't think the Else comment is useful here.

>>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:597
>> +	    RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::mergeStatistic. Statement failed to bind
or domain was not found, error message: %{public}s", this,
m_database.lastErrorMsg());
> 
> What will go into this error message? Since we're making it public, we need
to make sure nothing sensitive can end up there.

We should switch all of these "%{public}" log entries that relate to
registrable domain strings to "%{private}" so they don't go into the permanent
log. The ID's are fine, since they are machine-specific and don't reveal
anything useful (unless you obtain the ITP database from the machine generating
the log).

>>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1303
>> +	    || m_updateGrandfatheredStatement.bindText(2, domain.string()) !=
SQLITE_OK
> 
> Could this 2 be made a constant?

Whoops! I'm surprised we didn't see errors from binding text to the integer
valued parameter. I guess we don't hit this code often.

John: I don't think creating labels for these is super-useful. These are the
arguments in the Query (parameter 1, 2, etc.) Our general practice hasn't been
to create labels for these in our other database code, since each SQL statement
would then expand into a declaration for the statement, plus a declaration for
each parameter.

Kate: Maybe check with Brady to see if we have a SQL statement class that might
wrap all of this stuff up in some package that would help us maintain integrity
between statements and arguments. Might be a good idea for a future refinement.

>>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1792
>> +	    || m_updateDataRecordsRemovedStatement.bindText(2, domain.string())
!= SQLITE_OK
> 
> Same 2 again?

John: It's the second parameter of the SQL statement. Which is likely not the
same for 'm_updateDataRecordsRemovedStatement' as it was for
'm_updateTimesAccessedDueToFirstPartyInteraction'

>>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:267
>> +	    if (!m_statisticsStore)
> 
> Why is the !is<ResourceLoadStatisticsMemoryStore>(*m_statisticsStore) check
no longer needed?

That is the point of this entire patch -- to get the updates into the database!
:-)

>> Source/WebKit/NetworkProcess/NetworkProcess.h:245
>> +	void mergeStatisticForTesting(PAL::SessionID, const RegistrableDomain&,
const TopFrameDomain&, Seconds, bool, Seconds, bool isGrandfathered, bool,
bool, unsigned, CompletionHandler<void()>&&);
> 
> Three of the bool parameters don't have names. I think they should since the
header declares the interface. Same thing for the two Seconds and the unsigned.
> 
> Or could we use a named struct here?

Agree with both of these points.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:116
>> +	void mergeStatisticForTesting(PAL::SessionID, const RegistrableDomain&,
const TopFrameDomain&, Seconds, bool, Seconds, bool, bool, bool, unsigned,
CompletionHandler<void()>&&);
> 
> The bools and Seconds parameters, and the unsigned parameter should be named.
> 
> Or could we use a named struct here?

+1