[webkit-reviews] review denied: [Bug 195420] Enable LayoutTests using ResourceLoadStatistics SQLite backend : [Attachment 379274] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Katherine_cheney at apple.com's
request for review:
Bug 195420: Enable LayoutTests using ResourceLoadStatistics SQLite backend
https://bugs.webkit.org/show_bug.cgi?id=195420

Attachment 379274: Patch

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




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

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

I think this looks great! I did realize while reviewing that we log a number of
errors that could leak private data in the form of visited domains. Please
switch to using RELEASE_LOG_IF_ALLOWED for messages that involve the domain
strings. r- to fix the log statements, but everything else looks great!

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:389
>	   RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::m_insertDomainRelationshipStatement failed
to bind, error message: %{public}s", this, m_database.lastErrorMsg());

This might leak private data (the secondDomain.string() could be interesting to
someone). I recommend switching to RELEASE_LOG_IF_ALLOWED (see later review
comment for details).

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:394
> +    bool relationshipExists = !!statement.getColumnInt(0);

Oh boy. I think I did that. :-|

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:451
>	   RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::domainIDFromString failed, error message:
%{public}s", this, m_database.lastErrorMsg());

RELEASE_LOG_IF_ALLOWED

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:579
> +	   RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::recursivelyFindNonPrevalentDomainsThatRedi
rectedToThisDomain failed, error message: %{public}s", this,
m_database.lastErrorMsg());

This could be a privacy issue, since the JOIN result will contain the strings
of the visited domains.

Please switch to 'RELEASE_LOG_IF_ALLOWED"

(see WebCore/Modules/applepay/PaymentCoordinator.cpp for an example)

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:981
> +    if (m_mostRecentUserInteractionStatement.bindInt(1, hadUserInteraction)
!= SQLITE_OK

Good catch!

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1024
> +	   RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::m_hadUserInteractionStatement failed,
error message: %{public}s", this, m_database.lastErrorMsg());

RELEASE_LOG_IF_ALLOWED (because of domain.string())

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1121
>	   RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::predicateValueForDomain failed to bind,
error message: %{public}s", this, m_database.lastErrorMsg());

Ditto RELEASE_LOG_IF_ALLOWED, since the log message might show the domain that
was bound to the predicate.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1291
> +	   RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::ensureResourceStatisticsForRegistrableDoma
in failed, error message: %{public}s", this, m_database.lastErrorMsg());

RELEASE_LOG_IF_ALLOWED

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1346
> +	   || statement.bindText(1, domain.string()) != SQLITE_OK

Check error messages related to this statement (domain.string()) and use
RELEASE_LOG_IF_ALLOWED if logging.