REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should be read-only in ephemeral sessions

• Component: WebKit2
• Status: RESOLVED FIXED

[Michael Catanzaro]

GTK API test /webkit2/WebKitWebsiteData/ephemeral is broken:

ASSERTION FAILED: !storagePath.isEmpty()
../../Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp(147) : void WebKit::ResourceLoadStatisticsPersistentStorage::monitorDirectoryForNewStatistics()

Looks like this probably regressed in r219530 "Monitor directory for new statistics files after a delete operation" and we just didn't notice until now. Problem is WebResourceLoadStatisticsStore creates a ResourceLoadStatisticsPersistentStorage object unconditionally. ResourceLoadStatisticsPersistentStorage asserts that storagePath is not empty, but it's intentionally empty in ephemeral mode.

The proper fix is not obvious to me, but perhaps teaching WebResourceLoadStatisticsStore to not create a ResourceLoadStatisticsPersistentStorage in this case makes more sense than teaching ResourceLoadStatisticsPersistentStorage to be not persistent.

[Carlos Garcia Campos]

I can't reproduce this and the test is passing in the bots too.

[Michael Catanzaro]

I can reproduce 100%. The test is failing on our debug bot. It won't fail on the release bot because it's not a release assert.

I'm adding a debug skip expectation for this test, because the solution does not seem simple.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> I'm adding a debug skip expectation for this test, because the solution does
> not seem simple.

Since this bug affects Mac... when fixed, please remove the expectation near the top of Tools/Scripts/run-gtk-tests.

[Michael Catanzaro]

Committed r226364: <https://trac.webkit.org/changeset/226364>

[Michael Catanzaro]

Reopening.

[Radar WebKit Bug Importer]

<rdar://problem/36277541>

[Brent Fulgham]

This is actually tracked here:
<rdar://problem/36116604>

[Brent Fulgham]

Created attachment 330961 [details]
Patch

[Brent Fulgham]

Created attachment 330998 [details]
Patch with tests

[Brent Fulgham]

Comment on attachment 330998 [details]
Patch with tests

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

> Source/WebKit/ChangeLog:3
> +        REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should not be used in ephemeral sessions

I think a better title is "ResourceLoadStatisticsPersistentStorage should be read-only in ephemeral sessions". I'll fix that before landing.

[Chris Dumez]

Comment on attachment 330998 [details]
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(ForceImmediateWrite) 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.

[Chris Dumez]

Comment on attachment 330998 [details]
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?

Also, it would be better to actually rely on the session being ephemeral (we have this information in WebKit) rather than having a path or not, to set the ReadOnly mode.

[Brent Fulgham]

Created attachment 331140 [details]
Revised with Chris's comments.

[Chris Dumez]

Comment on attachment 331140 [details]
Revised with Chris's comments.

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

r=me with comment.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:97
> +    const_cast<Configuration*>(&m_configuration)->resourceLoadStatisticsDirectory = API::WebsiteDataStore::defaultResourceLoadStatisticsDirectory();

This looks a little ugly. Can we use instead:
struct Configuration {
  // ...
  String resourceLoadStatisticsDirectory {API::WebsiteDataStore::defaultResourceLoadStatisticsDirectory() };
}

in the header?

or add a default constructor to Configuration which always sets resourceLoadStatisticsDirectory.

[Brent Fulgham]

Created attachment 331141 [details]
Patch

[Brent Fulgham]

Created attachment 331147 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331147 [details]
Patch for landing

Rejecting attachment 331147 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 331147, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/6043135

[Brent Fulgham]

Created attachment 331149 [details]
Patch for landing

[Brent Fulgham]

Created attachment 331150 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 331150 [details]:

imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-v-framesize.html bug 181571 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 331150 [details]
Patch for landing

Clearing flags on attachment: 331150

Committed r226838: <https://trac.webkit.org/changeset/226838>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.