RESOLVED FIXED Bug 181136
REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should be read-only in ephemeral sessions
https://bugs.webkit.org/show_bug.cgi?id=181136
Summary REGRESSION(r219530): ResourceLoadStatisticsPersistentStorage should be read-o...
Michael Catanzaro
Reported 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.
Attachments
Patch (13.80 KB, patch)
2018-01-10 14:16 PST, Brent Fulgham
no flags
Patch with tests (19.25 KB, patch)
2018-01-10 17:39 PST, Brent Fulgham
cdumez: review-
Revised with Chris's comments. (20.19 KB, patch)
2018-01-11 16:46 PST, Brent Fulgham
no flags
Patch (20.62 KB, patch)
2018-01-11 16:57 PST, Brent Fulgham
no flags
Patch for landing (21.30 KB, patch)
2018-01-11 17:45 PST, Brent Fulgham
no flags
Patch for landing (21.37 KB, patch)
2018-01-11 17:58 PST, Brent Fulgham
no flags
Patch for landing (21.31 KB, patch)
2018-01-11 18:04 PST, Brent Fulgham
no flags
Carlos Garcia Campos
Comment 1 2018-01-03 02:29:09 PST
I can't reproduce this and the test is passing in the bots too.
Michael Catanzaro
Comment 2 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.
Michael Catanzaro
Comment 3 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.
Michael Catanzaro
Comment 4 2018-01-03 10:11:53 PST
Michael Catanzaro
Comment 5 2018-01-03 10:12:06 PST
Reopening.
Radar WebKit Bug Importer
Comment 6 2018-01-03 10:12:32 PST
Brent Fulgham
Comment 7 2018-01-10 14:04:09 PST
This is actually tracked here: <rdar://problem/36116604>
Brent Fulgham
Comment 8 2018-01-10 14:16:31 PST
Brent Fulgham
Comment 9 2018-01-10 17:39:27 PST
Created attachment 330998 [details] Patch with tests
Brent Fulgham
Comment 10 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.
Chris Dumez
Comment 11 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.
Chris Dumez
Comment 12 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.
Brent Fulgham
Comment 13 2018-01-11 16:46:49 PST
Created attachment 331140 [details] Revised with Chris's comments.
Chris Dumez
Comment 14 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.
Brent Fulgham
Comment 15 2018-01-11 16:57:20 PST
Brent Fulgham
Comment 16 2018-01-11 17:45:39 PST
Created attachment 331147 [details] Patch for landing
WebKit Commit Bot
Comment 17 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
Brent Fulgham
Comment 18 2018-01-11 17:58:53 PST
Created attachment 331149 [details] Patch for landing
Brent Fulgham
Comment 19 2018-01-11 18:04:28 PST
Created attachment 331150 [details] Patch for landing
WebKit Commit Bot
Comment 20 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.
WebKit Commit Bot
Comment 21 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>
WebKit Commit Bot
Comment 22 2018-01-11 19:13:13 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.