[webkit-reviews] review denied: [Bug 181136] REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should be read-only in ephemeral sessions : [Attachment 330998] Patch with tests

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 181136: REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should
be read-only in ephemeral sessions
https://bugs.webkit.org/show_bug.cgi?id=181136

Attachment 330998: Patch with tests

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




--- Comment #11 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 330998
  --> https://bugs.webkit.org/attachment.cgi?id=330998
Patch with tests

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

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:88
> +    if (storageDirectoryPath.isEmpty())

Seems weird to be dealing with an empty path here and to be calling into
API::WebsiteDataStore:: from this class. I believe we usually make sure path
are properly initialized early on. You probably want to update:
- ProcessPoolConfiguration::createWithLegacyOptions() to set
configuration->m_resourceLoadStatisticsDirectory

Or wouldn't this work for some reason?

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:102
> +void ResourceLoadStatisticsPersistentStorage::stopAsyncWriteTimer()

Not sure this is needed, see comment below.

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:44
> +    enum class PersistentStorageType {

I think this is more a Mode than a Type. However, unless we expect more modes,
I would suggest using enum class IsReadyOnly { No, Yes };

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:50
> +    static Ref<ResourceLoadStatisticsPersistentStorage>
create(WebResourceLoadStatisticsStore&, const String& storageDirectoryPath);

I do not believe we want this factory, see comment below.

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:91
> +    PersistentStorageType m_storageType { PersistentStorageType::ReadWrite
};

Since it is always set by the constructor, not sure it is worth having a
default initializer.

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:95
> +class ResourceLoadStatisticsMutablePersistentStorage : public
ResourceLoadStatisticsPersistentStorage {

final

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:103
> +class ResourceLoadStatisticsReadOnlyPersistentStorage : public
ResourceLoadStatisticsPersistentStorage {

final

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:108
> +	   stopAsyncWriteTimer();

Why is this needed? This timer is only started in
ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore(ForceImmedi
ateWrite) but you already override this function to do nothing.

> Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h:112
> +    void scheduleOrWriteMemoryStore(ForceImmediateWrite) override

This looks a bit odd to override to do nothing. We may want to either:
1.  have those functions pure virtual in the base class.
2. not use polymorphism at all and rely on the m_storageType data member in
ResourceLoadStatisticsPersistentStorage for these methods to be no-ops.

Personally, I'd go with 2. as it seems simpler.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:195
> +    Ref<ResourceLoadStatisticsPersistentStorage> m_persistentStorage;

I believe this causes a leak. The ref() method in
ResourceLoadStatisticsPersistentStorage ref's the
WebResourceLoadStatisticsStore, since it is its owner.
Therefore, my having WebResourceLoadStatisticsStore::m_persistentStorage be a
RefPtr, you effectively have WebResourceLoadStatisticsStore ref itself via the
ResourceLoadStatisticsPersistentStorage.