Bug 209253 – Handle failed ITP Database insert attempts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

Failure to successfully insert observed domains into the ITP Database should not cause crashes, and should be handled gracefully in case future code relies on the domain's presence in the database.

[Kate Cheney]

<rdar://problem/58886756>

[Kate Cheney]

Created attachment 393915 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 393915 [details]
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:215
> -    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;

[Kate Cheney]

(In reply to David Kilzer (:ddkilzer) from comment #3)
> Comment on attachment 393915 [details]
> 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.

Good idea
> 
> 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?

Actually, I don't even think we need an early return here. We should definitely check the result, but then we can do a best effort insert (even if it's no domains).
> 
> > 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:215
> > -    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;

[Kate Cheney]

Created attachment 394031 [details]
Patch for landing

[EWS]

Committed r258738: <https://trac.webkit.org/changeset/258738>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394031 [details].