WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r226364
: <
https://trac.webkit.org/changeset/226364
>
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
<
rdar://problem/36277541
>
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
Created
attachment 330961
[details]
Patch
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
Created
attachment 331141
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug