- Revision
- 233092
- Author
- [email protected]
- Date
- 2018-06-22 13:16:31 -0700 (Fri, 22 Jun 2018)
Log Message
Crash under WebResourceLoadStatisticsStore::mergeStatistics(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)
https://bugs.webkit.org/show_bug.cgi?id=186905
<rdar://problem/41266775>
Reviewed by Brent Fulgham.
I believe the crash was caused by the WebResourceLoadStatisticsStore object being dead
when mergeStatistics() is called. In particular, the crash was happening when the
ResourceLoadStatisticsPersistentStorage's FileMonitor would detect a file change and
we would re-sync statistics from the disk. The FileMonitor's lambda function was
capturing |this| without ref'ing it, and the FileMonitor monitors the disk and
calls the lambda on the background queue, while it gets destroyed on the main thread.
To make lifetime management less complex, the following changes were made:
- The ResourceLoadStatisticsPersistentStorage object is now always constructed / used
and destroyed on the background queue. We no longer have to worry about being on
the right thread in a given method.
- Now that ResourceLoadStatisticsPersistentStorage is always used from the background
queue and no longer needs to be thread-safe, drop its ref() / deref() methods and
use weak pointers instead to make sure the ResourceLoadStatisticsPersistentStorage
is still alive when a lamdba gets called on the background queue.
- For write scheduling use WorkQueue::dispatchAfter() and a WeakPtr instead of a
RunLoop::Timer. This is more convenient to use as the RunLoop::Timer has to be used
on the main thread.
* UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:
(WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage):
(WebKit::ResourceLoadStatisticsPersistentStorage::~ResourceLoadStatisticsPersistentStorage):
(WebKit::ResourceLoadStatisticsPersistentStorage::startMonitoringDisk):
(WebKit::ResourceLoadStatisticsPersistentStorage::monitorDirectoryForNewStatistics):
(WebKit::ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore):
* UIProcess/ResourceLoadStatisticsPersistentStorage.h:
* UIProcess/WebResourceLoadStatisticsStore.cpp:
(WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
(WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
(WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore):
(WebKit::WebResourceLoadStatisticsStore::processStatisticsAndDataRecords):
(WebKit::WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData):
(WebKit::WebResourceLoadStatisticsStore::applicationWillTerminate):
(WebKit::WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent):
* UIProcess/WebResourceLoadStatisticsStore.h:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (233091 => 233092)
--- trunk/Source/WebKit/ChangeLog 2018-06-22 20:06:40 UTC (rev 233091)
+++ trunk/Source/WebKit/ChangeLog 2018-06-22 20:16:31 UTC (rev 233092)
@@ -1,3 +1,47 @@
+2018-06-22 Chris Dumez <[email protected]>
+
+ Crash under WebResourceLoadStatisticsStore::mergeStatistics(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)
+ https://bugs.webkit.org/show_bug.cgi?id=186905
+ <rdar://problem/41266775>
+
+ Reviewed by Brent Fulgham.
+
+ I believe the crash was caused by the WebResourceLoadStatisticsStore object being dead
+ when mergeStatistics() is called. In particular, the crash was happening when the
+ ResourceLoadStatisticsPersistentStorage's FileMonitor would detect a file change and
+ we would re-sync statistics from the disk. The FileMonitor's lambda function was
+ capturing |this| without ref'ing it, and the FileMonitor monitors the disk and
+ calls the lambda on the background queue, while it gets destroyed on the main thread.
+
+ To make lifetime management less complex, the following changes were made:
+ - The ResourceLoadStatisticsPersistentStorage object is now always constructed / used
+ and destroyed on the background queue. We no longer have to worry about being on
+ the right thread in a given method.
+ - Now that ResourceLoadStatisticsPersistentStorage is always used from the background
+ queue and no longer needs to be thread-safe, drop its ref() / deref() methods and
+ use weak pointers instead to make sure the ResourceLoadStatisticsPersistentStorage
+ is still alive when a lamdba gets called on the background queue.
+ - For write scheduling use WorkQueue::dispatchAfter() and a WeakPtr instead of a
+ RunLoop::Timer. This is more convenient to use as the RunLoop::Timer has to be used
+ on the main thread.
+
+ * UIProcess/ResourceLoadStatisticsPersistentStorage.cpp:
+ (WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage):
+ (WebKit::ResourceLoadStatisticsPersistentStorage::~ResourceLoadStatisticsPersistentStorage):
+ (WebKit::ResourceLoadStatisticsPersistentStorage::startMonitoringDisk):
+ (WebKit::ResourceLoadStatisticsPersistentStorage::monitorDirectoryForNewStatistics):
+ (WebKit::ResourceLoadStatisticsPersistentStorage::scheduleOrWriteMemoryStore):
+ * UIProcess/ResourceLoadStatisticsPersistentStorage.h:
+ * UIProcess/WebResourceLoadStatisticsStore.cpp:
+ (WebKit::WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore):
+ (WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
+ (WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore):
+ (WebKit::WebResourceLoadStatisticsStore::processStatisticsAndDataRecords):
+ (WebKit::WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData):
+ (WebKit::WebResourceLoadStatisticsStore::applicationWillTerminate):
+ (WebKit::WebResourceLoadStatisticsStore::scheduleClearInMemoryAndPersistent):
+ * UIProcess/WebResourceLoadStatisticsStore.h:
+
2018-06-21 Jer Noble <[email protected]>
CRASH in WebCore::VideoFullscreenInterfaceMac::~VideoFullscreenInterfaceMac()
Modified: trunk/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp (233091 => 233092)
--- trunk/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp 2018-06-22 20:06:40 UTC (rev 233091)
+++ trunk/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.cpp 2018-06-22 20:16:31 UTC (rev 233092)
@@ -34,7 +34,6 @@
#include <WebCore/SharedBuffer.h>
#include <wtf/RunLoop.h>
#include <wtf/WorkQueue.h>
-#include <wtf/threads/BinarySemaphore.h>
namespace WebKit {
@@ -85,14 +84,10 @@
ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage(WebResourceLoadStatisticsStore& store, const String& storageDirectoryPath, IsReadOnly isReadOnly)
: m_memoryStore(store)
, m_storageDirectoryPath(storageDirectoryPath)
- , m_asyncWriteTimer(RunLoop::main(), this, &ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired)
, m_isReadOnly(isReadOnly)
{
-}
+ ASSERT(!RunLoop::isMain());
-void ResourceLoadStatisticsPersistentStorage::initialize()
-{
- ASSERT(!RunLoop::isMain());
populateMemoryStoreFromDisk();
startMonitoringDisk();
}
@@ -99,7 +94,10 @@
ResourceLoadStatisticsPersistentStorage::~ResourceLoadStatisticsPersistentStorage()
{
- ASSERT(!m_hasPendingWrite);
+ ASSERT(!RunLoop::isMain());
+
+ if (m_hasPendingWrite)
+ writeMemoryStoreToDisk();
}
String ResourceLoadStatisticsPersistentStorage::storageDirectoryPath() const
@@ -126,8 +124,11 @@
if (resourceLogPath.isEmpty())
return;
- m_fileMonitor = std::make_unique<FileMonitor>(resourceLogPath, m_memoryStore.statisticsQueue(), [this] (FileMonitor::FileChangeType type) {
+ m_fileMonitor = std::make_unique<FileMonitor>(resourceLogPath, m_memoryStore.statisticsQueue(), [this, weakThis = makeWeakPtr(*this)] (FileMonitor::FileChangeType type) {
ASSERT(!RunLoop::isMain());
+ if (!weakThis)
+ return;
+
switch (type) {
case FileMonitor::FileChangeType::Modification:
refreshMemoryStoreFromDisk();
@@ -143,6 +144,8 @@
void ResourceLoadStatisticsPersistentStorage::monitorDirectoryForNewStatistics()
{
+ ASSERT(!RunLoop::isMain());
+
String storagePath = storageDirectoryPath();
ASSERT(!storagePath.isEmpty());
@@ -237,15 +240,6 @@
m_memoryStore.logTestingEvent(ASCIILiteral("PopulatedWithoutGrandfathering"));
}
-void ResourceLoadStatisticsPersistentStorage::asyncWriteTimerFired()
-{
- ASSERT(RunLoop::isMain());
- RELEASE_ASSERT(m_isReadOnly != IsReadOnly::Yes);
- m_memoryStore.statisticsQueue().dispatch([this] () mutable {
- writeMemoryStoreToDisk();
- });
-}
-
void ResourceLoadStatisticsPersistentStorage::writeMemoryStoreToDisk()
{
ASSERT(!RunLoop::isMain());
@@ -292,8 +286,9 @@
if (!m_hasPendingWrite) {
m_hasPendingWrite = true;
Seconds delay = minimumWriteInterval - timeSinceLastWrite + 1_s;
- RunLoop::main().dispatch([this, protectedThis = makeRef(*this), delay] {
- m_asyncWriteTimer.startOneShot(delay);
+ m_memoryStore.statisticsQueue().dispatchAfter(delay, [weakThis = makeWeakPtr(*this)] {
+ if (weakThis)
+ weakThis->writeMemoryStoreToDisk();
});
}
return;
@@ -315,36 +310,6 @@
RELEASE_LOG_ERROR(ResourceLoadStatistics, "ResourceLoadStatisticsPersistentStorage: Unable to delete statistics file: %s", filePath.utf8().data());
}
-void ResourceLoadStatisticsPersistentStorage::finishAllPendingWorkSynchronously()
-{
- if (m_isReadOnly == IsReadOnly::Yes) {
- RELEASE_ASSERT(!m_asyncWriteTimer.isActive());
- return;
- }
-
- m_asyncWriteTimer.stop();
-
- BinarySemaphore semaphore;
- // Make sure any pending work in our queue is finished before we terminate.
- m_memoryStore.statisticsQueue().dispatch([&semaphore, this] {
- // Write final file state to disk.
- if (m_hasPendingWrite)
- writeMemoryStoreToDisk();
- semaphore.signal();
- });
- semaphore.wait(WallTime::infinity());
-}
-
-void ResourceLoadStatisticsPersistentStorage::ref()
-{
- m_memoryStore.ref();
-}
-
-void ResourceLoadStatisticsPersistentStorage::deref()
-{
- m_memoryStore.deref();
-}
-
#if !PLATFORM(IOS)
void ResourceLoadStatisticsPersistentStorage::excludeFromBackup() const
{
Modified: trunk/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h (233091 => 233092)
--- trunk/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h 2018-06-22 20:06:40 UTC (rev 233091)
+++ trunk/Source/WebKit/UIProcess/ResourceLoadStatisticsPersistentStorage.h 2018-06-22 20:16:31 UTC (rev 233092)
@@ -29,6 +29,7 @@
#include <wtf/MonotonicTime.h>
#include <wtf/RunLoop.h>
#include <wtf/WallTime.h>
+#include <wtf/WeakPtr.h>
#include <wtf/text/WTFString.h>
namespace WebCore {
@@ -39,24 +40,16 @@
class WebResourceLoadStatisticsStore;
-class ResourceLoadStatisticsPersistentStorage {
+// Can only be constructed / destroyed / used from the WebResourceLoadStatisticsStore's statistic queue.
+class ResourceLoadStatisticsPersistentStorage : public CanMakeWeakPtr<ResourceLoadStatisticsPersistentStorage> {
public:
enum class IsReadOnly { No, Yes };
ResourceLoadStatisticsPersistentStorage(WebResourceLoadStatisticsStore&, const String& storageDirectoryPath, IsReadOnly);
~ResourceLoadStatisticsPersistentStorage();
- void initialize();
void clear();
- void finishAllPendingWorkSynchronously();
-
- void ref();
- void deref();
-
- enum class ForceImmediateWrite {
- No,
- Yes,
- };
+ enum class ForceImmediateWrite { No, Yes, };
void scheduleOrWriteMemoryStore(ForceImmediateWrite);
private:
@@ -71,11 +64,9 @@
void populateMemoryStoreFromDisk();
void excludeFromBackup() const;
void refreshMemoryStoreFromDisk();
- void asyncWriteTimerFired();
WebResourceLoadStatisticsStore& m_memoryStore;
const String m_storageDirectoryPath;
- RunLoop::Timer<ResourceLoadStatisticsPersistentStorage> m_asyncWriteTimer;
std::unique_ptr<WebCore::FileMonitor> m_fileMonitor;
WallTime m_lastStatisticsFileSyncTime;
MonotonicTime m_lastStatisticsWriteTime;
Modified: trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp (233091 => 233092)
--- trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp 2018-06-22 20:06:40 UTC (rev 233091)
+++ trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp 2018-06-22 20:16:31 UTC (rev 233092)
@@ -46,6 +46,7 @@
#if !RELEASE_LOG_DISABLED
#include <wtf/text/StringBuilder.h>
#endif
+#include <wtf/threads/BinarySemaphore.h>
namespace WebKit {
using namespace WebCore;
@@ -165,7 +166,6 @@
WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(const String& resourceLoadStatisticsDirectory, Function<void(const String&)>&& testingCallback, bool isEphemeral, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&& updatePrevalentDomainsToPartitionOrBlockCookiesHandler, HasStorageAccessForFrameHandler&& hasStorageAccessForFrameHandler, GrantStorageAccessHandler&& grantStorageAccessHandler, RemoveAllStorageAccessHandler&& removeAllStorageAccessHandler, RemovePrevalentDomainsHandler&& removeDomainsHandler)
: m_statisticsQueue(WorkQueue::create("WebResourceLoadStatisticsStore Process Data Queue", WorkQueue::Type::Serial, WorkQueue::QOS::Utility))
- , m_persistentStorage(*this, resourceLoadStatisticsDirectory, isEphemeral ? ResourceLoadStatisticsPersistentStorage::IsReadOnly::Yes : ResourceLoadStatisticsPersistentStorage::IsReadOnly::No)
, m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler(WTFMove(updatePrevalentDomainsToPartitionOrBlockCookiesHandler))
, m_hasStorageAccessForFrameHandler(WTFMove(hasStorageAccessForFrameHandler))
, m_grantStorageAccessHandler(WTFMove(grantStorageAccessHandler))
@@ -180,8 +180,8 @@
registerUserDefaultsIfNeeded();
#endif
- m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this)] {
- m_persistentStorage.initialize();
+ m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), isEphemeral, resourceLoadStatisticsDirectory = resourceLoadStatisticsDirectory.isolatedCopy()] {
+ m_persistentStorage = std::make_unique<ResourceLoadStatisticsPersistentStorage>(*this, resourceLoadStatisticsDirectory, isEphemeral ? ResourceLoadStatisticsPersistentStorage::IsReadOnly::Yes : ResourceLoadStatisticsPersistentStorage::IsReadOnly::No);
includeTodayAsOperatingDateIfNecessary();
});
@@ -195,9 +195,24 @@
WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
{
- m_persistentStorage.finishAllPendingWorkSynchronously();
+ flushAndDestroyPersistentStore();
}
+void WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
+{
+ if (!m_persistentStorage)
+ return;
+
+ // Make sure we destroy the persistent store on the background queue and wait for it to die
+ // synchronously since it has a C++ reference to us.
+ BinarySemaphore semaphore;
+ m_statisticsQueue->dispatch([&semaphore, this] {
+ m_persistentStorage = nullptr;
+ semaphore.signal();
+ });
+ semaphore.wait(WallTime::infinity());
+}
+
#if !RELEASE_LOG_DISABLED
static void appendWithDelimiter(StringBuilder& builder, const String& domain, bool isFirstItem)
{
@@ -341,7 +356,8 @@
ASSERT(!RunLoop::isMain());
pruneStatisticsIfNeeded();
- m_persistentStorage.scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
+ if (m_persistentStorage)
+ m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
RunLoop::main().dispatch([] {
WebProcessProxy::notifyPageStatisticsAndDataRecordsProcessed();
@@ -352,7 +368,8 @@
ASSERT(!RunLoop::isMain());
pruneStatisticsIfNeeded();
- m_persistentStorage.scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
+ if (m_persistentStorage)
+ m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::No);
});
}
}
@@ -534,7 +551,8 @@
statistic.grandfathered = true;
}
m_endOfGrandfatheringTimestamp = WallTime::now() + m_parameters.grandfatheringTime;
- m_persistentStorage.scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::Yes);
+ if (m_persistentStorage)
+ m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::Yes);
callback();
logTestingEvent(ASCIILiteral("Grandfathered"));
});
@@ -554,7 +572,7 @@
void WebResourceLoadStatisticsStore::applicationWillTerminate()
{
- m_persistentStorage.finishAllPendingWorkSynchronously();
+ flushAndDestroyPersistentStore();
}
void WebResourceLoadStatisticsStore::performDailyTasks()
@@ -1050,7 +1068,8 @@
ASSERT(RunLoop::isMain());
m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), shouldGrandfather, completionHandler = WTFMove(completionHandler)] () mutable {
clearInMemory();
- m_persistentStorage.clear();
+ if (m_persistentStorage)
+ m_persistentStorage->clear();
if (shouldGrandfather == ShouldGrandfather::Yes)
grandfatherExistingWebsiteData(WTFMove(completionHandler));
Modified: trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h (233091 => 233092)
--- trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h 2018-06-22 20:06:40 UTC (rev 233091)
+++ trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h 2018-06-22 20:16:31 UTC (rev 233092)
@@ -200,6 +200,8 @@
void setDebugLogggingEnabled(bool enabled) { m_debugLoggingEnabled = enabled; }
void setStorageAccessPromptsEnabled(bool enabled) { m_storageAccessPromptsEnabled = enabled; }
+ void flushAndDestroyPersistentStore();
+
#if PLATFORM(COCOA)
void registerUserDefaultsIfNeeded();
#endif
@@ -225,7 +227,7 @@
ResourceLoadStatisticsClassifier m_resourceLoadStatisticsClassifier;
#endif
Ref<WTF::WorkQueue> m_statisticsQueue;
- ResourceLoadStatisticsPersistentStorage m_persistentStorage;
+ std::unique_ptr<ResourceLoadStatisticsPersistentStorage> m_persistentStorage;
Vector<OperatingDate> m_operatingDates;
UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler;
