Bug 181136 - REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should be read-only in ephemeral sessions
Summary: REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should be read-o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-22 14:07 PST by Michael Catanzaro
Modified: 2018-01-11 19:13 PST (History)
8 users (show)

See Also:


Attachments
Patch (13.80 KB, patch)
2018-01-10 14:16 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch with tests (19.25 KB, patch)
2018-01-10 17:39 PST, Brent Fulgham
cdumez: review-
Details | Formatted Diff | Diff
Revised with Chris's comments. (20.19 KB, patch)
2018-01-11 16:46 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (20.62 KB, patch)
2018-01-11 16:57 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (21.30 KB, patch)
2018-01-11 17:45 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (21.37 KB, patch)
2018-01-11 17:58 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (21.31 KB, patch)
2018-01-11 18:04 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-12-22 14:07:17 PST
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.
Comment 1 Carlos Garcia Campos 2018-01-03 02:29:09 PST
I can't reproduce this and the test is passing in the bots too.
Comment 2 Michael Catanzaro 2018-01-03 08:54:41 PST
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.
Comment 3 Michael Catanzaro 2018-01-03 09:03:36 PST
(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.
Comment 4 Michael Catanzaro 2018-01-03 10:11:53 PST
Committed r226364: <https://trac.webkit.org/changeset/226364>
Comment 5 Michael Catanzaro 2018-01-03 10:12:06 PST
Reopening.
Comment 6 Radar WebKit Bug Importer 2018-01-03 10:12:32 PST
<rdar://problem/36277541>
Comment 7 Brent Fulgham 2018-01-10 14:04:09 PST
This is actually tracked here:
<rdar://problem/36116604>
Comment 8 Brent Fulgham 2018-01-10 14:16:31 PST
Created attachment 330961 [details]
Patch
Comment 9 Brent Fulgham 2018-01-10 17:39:27 PST
Created attachment 330998 [details]
Patch with tests
Comment 10 Brent Fulgham 2018-01-10 20:21:19 PST
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.
Comment 11 Chris Dumez 2018-01-11 13:56:30 PST
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.
Comment 12 Chris Dumez 2018-01-11 14:15:00 PST
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.
Comment 13 Brent Fulgham 2018-01-11 16:46:49 PST
Created attachment 331140 [details]
Revised with Chris's comments.
Comment 14 Chris Dumez 2018-01-11 16:55:21 PST
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.
Comment 15 Brent Fulgham 2018-01-11 16:57:20 PST
Created attachment 331141 [details]
Patch
Comment 16 Brent Fulgham 2018-01-11 17:45:39 PST
Created attachment 331147 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2018-01-11 17:47:22 PST
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
Comment 18 Brent Fulgham 2018-01-11 17:58:53 PST
Created attachment 331149 [details]
Patch for landing
Comment 19 Brent Fulgham 2018-01-11 18:04:28 PST
Created attachment 331150 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2018-01-11 19:12:41 PST
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: cdumez@apple.com and jer.noble@apple.com)
The commit-queue is continuing to process your patch.
Comment 21 WebKit Commit Bot 2018-01-11 19:13:11 PST
Comment on attachment 331150 [details]
Patch for landing

Clearing flags on attachment: 331150

Committed r226838: <https://trac.webkit.org/changeset/226838>
Comment 22 WebKit Commit Bot 2018-01-11 19:13:13 PST
All reviewed patches have been landed.  Closing bug.