[webkit-reviews] review granted: [Bug 209253] Handle failed ITP Database insert attempts : [Attachment 393915] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted
katherine_cheney at apple.com's request for review:
Bug 209253: Handle failed ITP Database insert attempts
https://bugs.webkit.org/show_bug.cgi?id=209253

Attachment 393915: Patch

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




--- Comment #3 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 393915
  --> https://bugs.webkit.org/attachment.cgi?id=393915
Patch

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

r=me

This seems fine, but you should probably update release logging so methods that
have more than one release log statement have a (slightly) different log
message.

Also, I'd suggest adding WARN_UNUSED_RETURN for the two methods I noted, and
recompile (to make sure you're not missing a return value check anywhere).

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1481
> +	       RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::grantStorageAccessInternal was not
completed due to failed insert attempt", this);

Might want to make this logging different so you know which statement failed in
ResourceLoadStatisticsDatabaseStore::grantStorageAccessInternal().

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1511
> +    for (auto& registrableDomain : domains) {
> +	   auto result =
ensureResourceStatisticsForRegistrableDomain(registrableDomain);
> +	   if (!result.second) {
> +	       RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::grandfatherDataForDomains was not
completed due to failed insert attempt", this);
> +	       return;
> +	   }
> +    }

Let's say the first domain fails.  Is it okay to assume all the other domains
will fail, or do you want to continue to iterate through all of them, and only
return early if some failed?

    bool insertionFailure = false;
    for (auto& registrableDomain : domains) {
	auto result =
ensureResourceStatisticsForRegistrableDomain(registrableDomain);
	if (!result.second) {
	    RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::grandfatherDataForDomains was not
completed due to failed insert attempt", this);
	    insertionFailure = true;
	}
    }
    if (insertionFailure)
	return;

Or maybe you only want to return early if no insertions succeeded?

    bool atLeastOneInsertionSucceeded = false;
    for (auto& registrableDomain : domains) {
	auto result =
ensureResourceStatisticsForRegistrableDomain(registrableDomain);
	if (!result.second) {
	    RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::grandfatherDataForDomains was not
completed due to failed insert attempt", this);
	} else
	    atLeastOneInsertionSucceeded = true;
    }
    if (!atLeastOneInsertionSucceeded)
	return;

Or is this the cause of the crash later, which is why you return early here?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1543
> +	       RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::ensurePrevalentResourcesForDebugMode was
not completed due to failed insert attempt", this);

Might want to make this logging different so you know which statement failed in
ResourceLoadStatisticsDatabaseStore::ensurePrevalentResourcesForDebugMode().

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1580
> +		       RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::logFrameNavigation was not completed due
to failed insert attempt", this);

Might want to make this logging different so you know which statement failed in
ResourceLoadStatisticsDatabaseStore::logFrameNavigation().

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1591
> +		   RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::logFrameNavigation was not completed due
to failed insert attempt", this);

Might want to make this logging different so you know which statement failed in
ResourceLoadStatisticsDatabaseStore::logFrameNavigation().

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:1974
> +	   RELEASE_LOG_ERROR_IF_ALLOWED(m_sessionID, "%p -
ResourceLoadStatisticsDatabaseStore::setSubresourceUnderTopFrameDomain was not
completed due to failed insert attempt", this);

Might want to make this logging different so you know which statement failed in
ResourceLoadStatisticsDatabaseStore::setSubresourceUnderTopFrameDomain().

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:2055
> -    insertObservedDomain(newObservation);
> +    auto result = insertObservedDomain(newObservation);

May want to add WARN_UNUSED_RETURN in the header declaration for
ResourceLoadStatisticsDatabaseStore::insertObservedDomain() as well.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:2
15
> -    std::pair<AddedRecord, unsigned>
ensureResourceStatisticsForRegistrableDomain(const RegistrableDomain&);
> +    std::pair<AddedRecord, Optional<unsigned>>
ensureResourceStatisticsForRegistrableDomain(const RegistrableDomain&);

You may want to add WARN_UNUSED_RETURN here (and recompile) to make sure return
values are checked:

    std::pair<AddedRecord, Optional<unsigned>>
ensureResourceStatisticsForRegistrableDomain(const RegistrableDomain&)
WARN_UNUSED_RETURN;