Resource load statistics: Cover further data records, count removed data records, and only fire handler when needed

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

Resource load statistics: Cover further data records, count removed data records, and only fire handler when needed.

[John Wilander]

Created attachment 294569 [details]
Patch

[Brent Fulgham]

Martin (or other GTK people) -- can anyone help us understand why GTK is failing to build here?

[John Wilander]

Created attachment 294755 [details]
Patch

[John Wilander]

Added an explicit this-> to the call that the GTK build is complaining about.

[Brent Fulgham]

Comment on attachment 294755 [details]
Patch

Looks reasonable. Please get Andy to do final review due to WebKit2 content.

[Andy Estes]

Comment on attachment 294755 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +            All three functions now more conservative in calls to m_store->fireDataModificationHandler(). 

"are now more conservative"

> Source/WebCore/ChangeLog:15
> +            The only fire when an important statistic has changed or data records have previously been

"They only fire"

> Source/WebCore/loader/ResourceLoadStatistics.cpp:176
> +    // Keys missing from earlier formats should not cause the function to return false
> +    decoder.decodeUInt32("dataRecordsRemoved", dataRecordsRemoved);

This doesn't seem like a good idea. I believe ResourceLoadStatistics encoding/decoding happens in two places:

1. When reading from/writing to the plist on disk.
2. When communicating between the UI process and the Web process.

For #1, I suppose it's ok to decode an older format on disk by ignoring a missing key. But for #2 it's not ok, because a missing "dataRecordsRemoved" key could represent a corrupt message from a compromised process.

If we're going to support multiple formats, then we should add a "version" key to the structure so we can distinguish between an old format and a corrupted message.

[John Wilander]

Created attachment 294852 [details]
Patch

[John Wilander]

Created attachment 294853 [details]
Patch

[John Wilander]

(In reply to comment #6)
> Comment on attachment 294755 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294755&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +            All three functions now more conservative in calls to m_store->fireDataModificationHandler(). 
> 
> "are now more conservative"

Fixed.

> > Source/WebCore/ChangeLog:15
> > +            The only fire when an important statistic has changed or data records have previously been
> 
> "They only fire"

Fixed.

> > Source/WebCore/loader/ResourceLoadStatistics.cpp:176
> > +    // Keys missing from earlier formats should not cause the function to return false
> > +    decoder.decodeUInt32("dataRecordsRemoved", dataRecordsRemoved);
> 
> This doesn't seem like a good idea. I believe ResourceLoadStatistics
> encoding/decoding happens in two places:
> 
> 1. When reading from/writing to the plist on disk.
> 2. When communicating between the UI process and the Web process.
> 
> For #1, I suppose it's ok to decode an older format on disk by ignoring a
> missing key. But for #2 it's not ok, because a missing "dataRecordsRemoved"
> key could represent a corrupt message from a compromised process.
> 
> If we're going to support multiple formats, then we should add a "version"
> key to the structure so we can distinguish between an old format and a
> corrupted message.

That is a very good idea! Fixed.

Thanks, Andy! Please see updated patch.

[Andy Estes]

Comment on attachment 294853 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadStatistics.cpp:176
> +    if (!decoder.decodeUInt32("dataRecordsRemoved", dataRecordsRemoved) && version > 1)
> +        return false;

I'd prefer to see the version check broken out in its own statement for clarity:

    if (version < 2)
        return true;

    if (!decoder.decodeUInt32("dataRecordsRemoved", dataRecordsRemoved))
        return false;

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:90
> +    uint32_t version;

I'd make this an unsigned for consistency.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:94
> +    bool succeeded = decoder.decodeObjects("browsingStatistics", loadedStatistics, [this, version = WTFMove(version)](KeyedDecoder& decoderInner, ResourceLoadStatistics& statistics) {

You can just capture version by value (i.e. [this, version]). There's no benefit to moving a uint32_t.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:130
> +    if (dataTypesToRemove.isEmpty()) {
> +        dataTypesToRemove |= WebsiteDataType::Cookies;
> +        dataTypesToRemove |= WebsiteDataType::LocalStorage;
> +        dataTypesToRemove |= WebsiteDataType::IndexedDBDatabases;
> +        dataTypesToRemove |= WebsiteDataType::DiskCache;
> +        dataTypesToRemove |= WebsiteDataType::MemoryCache;
> +    }

Since dataTypesToRemove is only used by this function, you should declare it as a local static:

    static OptionSet<WebsiteDataType> dataTypesToRemove {
        WebsiteDataType::Cookies,
        WebsiteDataType::LocalStorage,
        WebsiteDataType::IndexedDBDatabases,
        WebsiteDataType::DiskCache,
        WebsiteDataType::MemoryCache
    };

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:157
> +            websiteDataStore.removeData(dataTypesToRemove, { WTFMove(dataRecords) }, [prevalentResourceDomainsWithDataRecords = WTFMove(prevalentResourceDomainsWithDataRecords), this] {

"{ WTFMove(dataRecords) } " is pretty contorted. The function takes a const reference, so it's ok to just pass dataRecords directly without moving it.

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:158
> +                this->coreStore().updateStatisticsForRemovedDataRecords(prevalentResourceDomainsWithDataRecords);

I don't think the "this->" is necessary.

[John Wilander]

Created attachment 295115 [details]
Patch

[John Wilander]

(In reply to comment #10)
> Comment on attachment 294853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294853&action=review
> 
> > Source/WebCore/loader/ResourceLoadStatistics.cpp:176
> > +    if (!decoder.decodeUInt32("dataRecordsRemoved", dataRecordsRemoved) && version > 1)
> > +        return false;
> 
> I'd prefer to see the version check broken out in its own statement for
> clarity:
> 
>     if (version < 2)
>         return true;
> 
>     if (!decoder.decodeUInt32("dataRecordsRemoved", dataRecordsRemoved))
>         return false;

Fixed.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:90
> > +    uint32_t version;
> 
> I'd make this an unsigned for consistency.

Fixed.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:94
> > +    bool succeeded = decoder.decodeObjects("browsingStatistics", loadedStatistics, [this, version = WTFMove(version)](KeyedDecoder& decoderInner, ResourceLoadStatistics& statistics) {
> 
> You can just capture version by value (i.e. [this, version]). There's no
> benefit to moving a uint32_t.

Fixed.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:130
> > +    if (dataTypesToRemove.isEmpty()) {
> > +        dataTypesToRemove |= WebsiteDataType::Cookies;
> > +        dataTypesToRemove |= WebsiteDataType::LocalStorage;
> > +        dataTypesToRemove |= WebsiteDataType::IndexedDBDatabases;
> > +        dataTypesToRemove |= WebsiteDataType::DiskCache;
> > +        dataTypesToRemove |= WebsiteDataType::MemoryCache;
> > +    }
> 
> Since dataTypesToRemove is only used by this function, you should declare it
> as a local static:
> 
>     static OptionSet<WebsiteDataType> dataTypesToRemove {
>         WebsiteDataType::Cookies,
>         WebsiteDataType::LocalStorage,
>         WebsiteDataType::IndexedDBDatabases,
>         WebsiteDataType::DiskCache,
>         WebsiteDataType::MemoryCache
>     };

Fixed.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:157
> > +            websiteDataStore.removeData(dataTypesToRemove, { WTFMove(dataRecords) }, [prevalentResourceDomainsWithDataRecords = WTFMove(prevalentResourceDomainsWithDataRecords), this] {
> 
> "{ WTFMove(dataRecords) } " is pretty contorted. The function takes a const
> reference, so it's ok to just pass dataRecords directly without moving it.

Fixed.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:158
> > +                this->coreStore().updateStatisticsForRemovedDataRecords(prevalentResourceDomainsWithDataRecords);
> 
> I don't think the "this->" is necessary.

Actually it is. Adding it fixed the GTK build.

[John Wilander]

Created attachment 295117 [details]
Patch

[John Wilander]

Moved dataTypesToRemove back out as a member variable since GTK didn't build with the local static one. Couldn't resolve it in the second lambda.

[WebKit Commit Bot]

Comment on attachment 295117 [details]
Patch

Clearing flags on attachment: 295117

Committed r208874: <http://trac.webkit.org/changeset/208874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.