Diff
Modified: trunk/Source/WebCore/ChangeLog (219218 => 219219)
--- trunk/Source/WebCore/ChangeLog 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Source/WebCore/ChangeLog 2017-07-06 21:45:04 UTC (rev 219219)
@@ -1,3 +1,32 @@
+2017-07-06 Chris Dumez <[email protected]>
+
+ FileMonitor should not be ref counted
+ https://bugs.webkit.org/show_bug.cgi?id=174166
+
+ Reviewed by Brent Fulgham.
+
+ Update FileMonitor to no longer be refcounted. It was previously easy to leak it
+ because the object would ref itself in various lambdas. The client would have to
+ explicitely call FileMonitor::stopMonitoring() which was fragile.
+
+ This patch also simplifies the code and API a bit since no longer actually
+ requires startMonitoring() / stopMonitoring() API.
+
+ No new tests, covered by API tests.
+
+ * platform/FileMonitor.cpp:
+ (WebCore::FileMonitor::FileMonitor):
+ (WebCore::FileMonitor::~FileMonitor):
+ (WebCore::FileMonitor::create): Deleted.
+ (WebCore::FileMonitor::startMonitoring): Deleted.
+ (WebCore::FileMonitor::stopMonitoring): Deleted.
+ * platform/FileMonitor.h:
+ * platform/cocoa/FileMonitorCocoa.mm:
+ (WebCore::FileMonitor::FileMonitor):
+ (WebCore::FileMonitor::~FileMonitor):
+ (WebCore::FileMonitor::startMonitoring): Deleted.
+ (WebCore::FileMonitor::stopMonitoring): Deleted.
+
2017-07-06 Matt Rajca <[email protected]>
Fix build with VIDEO support disabled.
Modified: trunk/Source/WebCore/platform/FileMonitor.cpp (219218 => 219219)
--- trunk/Source/WebCore/platform/FileMonitor.cpp 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Source/WebCore/platform/FileMonitor.cpp 2017-07-06 21:45:04 UTC (rev 219219)
@@ -28,33 +28,15 @@
namespace WebCore {
-Ref<FileMonitor> FileMonitor::create(const String& path, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler)
+#if !PLATFORM(COCOA)
+
+FileMonitor::FileMonitor(const String&, Ref<WorkQueue>&&, WTF::Function<void(FileChangeType)>&&)
{
- return adoptRef(*new FileMonitor(path, WTFMove(handlerQueue), WTFMove(modificationHandler)));
}
-
-FileMonitor::FileMonitor(const String& path, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler)
- : m_path(path)
- , m_modificationHandler(WTFMove(modificationHandler))
- , m_handlerQueue(WTFMove(handlerQueue))
-{
-}
-
+
FileMonitor::~FileMonitor()
{
- stopMonitoring();
}
-
-#if !PLATFORM(COCOA)
-void FileMonitor::startMonitoring()
-{
- // Do Nothing
-}
-
-void FileMonitor::stopMonitoring()
-{
- // Do Nothing
-}
#endif
Modified: trunk/Source/WebCore/platform/FileMonitor.h (219218 => 219219)
--- trunk/Source/WebCore/platform/FileMonitor.h 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Source/WebCore/platform/FileMonitor.h 2017-07-06 21:45:04 UTC (rev 219219)
@@ -26,7 +26,6 @@
#pragma once
#include <wtf/Function.h>
-#include <wtf/ThreadSafeRefCounted.h>
#include <wtf/WorkQueue.h>
#include <wtf/text/WTFString.h>
@@ -37,7 +36,7 @@
namespace WebCore {
-class FileMonitor : public ThreadSafeRefCounted<FileMonitor> {
+class FileMonitor {
public:
enum class FileChangeType {
Modification,
@@ -44,19 +43,10 @@
Removal
};
- WEBCORE_EXPORT static Ref<FileMonitor> create(const String&, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler);
+ WEBCORE_EXPORT FileMonitor(const String&, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler);
WEBCORE_EXPORT ~FileMonitor();
- WEBCORE_EXPORT void startMonitoring();
- WEBCORE_EXPORT void stopMonitoring();
-
private:
- FileMonitor(const String&, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler);
-
- String m_path;
- WTF::Function<void(FileChangeType)> m_modificationHandler;
- Ref<WTF::WorkQueue> m_handlerQueue;
-
#if USE(COCOA_EVENT_LOOP)
DispatchPtr<dispatch_source_t> m_platformMonitor;
#endif
Modified: trunk/Source/WebCore/platform/cocoa/FileMonitorCocoa.mm (219218 => 219219)
--- trunk/Source/WebCore/platform/cocoa/FileMonitorCocoa.mm 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Source/WebCore/platform/cocoa/FileMonitorCocoa.mm 2017-07-06 21:45:04 UTC (rev 219219)
@@ -28,8 +28,7 @@
#import "FileSystem.h"
#import "Logging.h"
-#import <dispatch/dispatch.h>
-#import <wtf/DispatchPtr.h>
+#import <wtf/BlockPtr.h>
namespace WebCore {
@@ -36,52 +35,49 @@
constexpr unsigned monitorMask = DISPATCH_VNODE_DELETE | DISPATCH_VNODE_WRITE | DISPATCH_VNODE_RENAME | DISPATCH_VNODE_REVOKE;
constexpr unsigned fileUnavailableMask = DISPATCH_VNODE_DELETE | DISPATCH_VNODE_RENAME | DISPATCH_VNODE_REVOKE;
-void FileMonitor::startMonitoring()
+FileMonitor::FileMonitor(const String& path, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler)
{
- if (m_platformMonitor)
+ if (path.isEmpty())
return;
- if (m_path.isEmpty())
+ if (!modificationHandler)
return;
- if (!m_modificationHandler)
- return;
-
- auto handle = openFile(m_path, OpenForEventsOnly);
+ auto handle = openFile(path, OpenForEventsOnly);
if (handle == invalidPlatformFileHandle) {
- RELEASE_LOG_ERROR(ResourceLoadStatistics, "Failed to open statistics file for monitoring: %s", m_path.utf8().data());
+ RELEASE_LOG_ERROR(ResourceLoadStatistics, "Failed to open statistics file for monitoring: %s", path.utf8().data());
return;
}
- m_platformMonitor = adoptDispatch(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle, monitorMask, m_handlerQueue->dispatchQueue()));
+ // The source (platformMonitor) retains the dispatch queue.
+ m_platformMonitor = adoptDispatch(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle, monitorMask, handlerQueue->dispatchQueue()));
LOG(ResourceLoadStatistics, "Creating monitor %p", m_platformMonitor.get());
- dispatch_source_set_event_handler(m_platformMonitor.get(), [this, protectedThis = makeRefPtr(this), fileMonitor = m_platformMonitor.get()] () mutable {
+ dispatch_source_set_event_handler(m_platformMonitor.get(), BlockPtr<void()>::fromCallable([modificationHandler = WTFMove(modificationHandler), fileMonitor = m_platformMonitor] {
// If this is getting called after the monitor was cancelled, just drop the notification.
- if (dispatch_source_testcancel(fileMonitor))
+ if (dispatch_source_testcancel(fileMonitor.get()))
return;
- unsigned flag = dispatch_source_get_data(fileMonitor);
- LOG(ResourceLoadStatistics, "File event %#X for monitor %p", flag, fileMonitor);
+ unsigned flag = dispatch_source_get_data(fileMonitor.get());
+ LOG(ResourceLoadStatistics, "File event %#X for monitor %p", flag, fileMonitor.get());
if (flag & fileUnavailableMask) {
- m_modificationHandler(FileChangeType::Removal);
- dispatch_source_cancel(fileMonitor);
+ modificationHandler(FileChangeType::Removal);
+ dispatch_source_cancel(fileMonitor.get());
} else {
ASSERT(flag & DISPATCH_VNODE_WRITE);
- m_modificationHandler(FileChangeType::Modification);
+ modificationHandler(FileChangeType::Modification);
}
- });
+ }).get());
- dispatch_source_set_cancel_handler(m_platformMonitor.get(), [fileMonitor = m_platformMonitor.get()] {
- auto handle = static_cast<PlatformFileHandle>(dispatch_source_get_handle(fileMonitor));
+ dispatch_source_set_cancel_handler(m_platformMonitor.get(), [handle] () mutable {
closeFile(handle);
});
dispatch_resume(m_platformMonitor.get());
}
-
-void FileMonitor::stopMonitoring()
+
+FileMonitor::~FileMonitor()
{
if (!m_platformMonitor)
return;
@@ -89,7 +85,6 @@
LOG(ResourceLoadStatistics, "Stopping monitor %p", m_platformMonitor.get());
dispatch_source_cancel(m_platformMonitor.get());
- m_platformMonitor = nullptr;
}
} // namespace WebCore
Modified: trunk/Source/WebKit2/ChangeLog (219218 => 219219)
--- trunk/Source/WebKit2/ChangeLog 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Source/WebKit2/ChangeLog 2017-07-06 21:45:04 UTC (rev 219219)
@@ -1,3 +1,17 @@
+2017-07-06 Chris Dumez <[email protected]>
+
+ FileMonitor should not be ref counted
+ https://bugs.webkit.org/show_bug.cgi?id=174166
+
+ Reviewed by Brent Fulgham.
+
+ Update code using FileMonitor to reflect API change.
+
+ * UIProcess/WebResourceLoadStatisticsStore.cpp:
+ (WebKit::WebResourceLoadStatisticsStore::startMonitoringStatisticsStorage):
+ (WebKit::WebResourceLoadStatisticsStore::stopMonitoringStatisticsStorage):
+ * UIProcess/WebResourceLoadStatisticsStore.h:
+
2017-07-06 Matt Rajca <[email protected]>
Fix build with VIDEO support disabled.
Modified: trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp (219218 => 219219)
--- trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp 2017-07-06 21:45:04 UTC (rev 219219)
@@ -434,7 +434,7 @@
if (resourceLogPath.isEmpty())
return;
- m_statisticsStorageMonitor = FileMonitor::create(resourceLogPath, m_statisticsQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
+ m_statisticsStorageMonitor = std::make_unique<FileMonitor>(resourceLogPath, m_statisticsQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
ASSERT(!RunLoop::isMain());
switch (type) {
case FileMonitor::FileChangeType::Modification:
@@ -446,19 +446,11 @@
break;
}
});
-
- m_statisticsStorageMonitor->startMonitoring();
}
void WebResourceLoadStatisticsStore::stopMonitoringStatisticsStorage()
{
ASSERT(!RunLoop::isMain());
- if (!m_statisticsStorageMonitor)
- return;
-
- // FIXME(174166): The FileMonitor captures itself, incrementing its refcount. Manually stopping the monitor shuts down the lambda holding the extra
- // reference, so the object will be cleaned up properly.
- m_statisticsStorageMonitor->stopMonitoring();
m_statisticsStorageMonitor = nullptr;
}
Modified: trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h (219218 => 219219)
--- trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h 2017-07-06 21:45:04 UTC (rev 219219)
@@ -145,7 +145,7 @@
ResourceLoadStatisticsClassifier m_resourceLoadStatisticsClassifier;
#endif
Ref<WTF::WorkQueue> m_statisticsQueue;
- RefPtr<WebCore::FileMonitor> m_statisticsStorageMonitor;
+ std::unique_ptr<WebCore::FileMonitor> m_statisticsStorageMonitor;
const String m_statisticsStoragePath;
WTF::WallTime m_lastStatisticsFileSyncTime;
RunLoop::Timer<WebResourceLoadStatisticsStore> m_telemetryOneShotTimer;
Modified: trunk/Tools/ChangeLog (219218 => 219219)
--- trunk/Tools/ChangeLog 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Tools/ChangeLog 2017-07-06 21:45:04 UTC (rev 219219)
@@ -1,3 +1,15 @@
+2017-07-06 Chris Dumez <[email protected]>
+
+ FileMonitor should not be ref counted
+ https://bugs.webkit.org/show_bug.cgi?id=174166
+
+ Reviewed by Brent Fulgham.
+
+ Update the API tests to reflect the API change.
+
+ * TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:
+ (TestWebKitAPI::TEST_F):
+
2017-07-06 Commit Queue <[email protected]>
Unreviewed, rolling out r219194.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp (219218 => 219219)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp 2017-07-06 21:22:48 UTC (rev 219218)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp 2017-07-06 21:45:04 UTC (rev 219219)
@@ -149,7 +149,7 @@
auto testQueue = WorkQueue::create("Test Work Queue");
- auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
+ auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
ASSERT(!RunLoop::isMain());
switch (type) {
case FileMonitor::FileChangeType::Modification:
@@ -160,7 +160,6 @@
break;
}
});
- monitor->startMonitoring();
testQueue->dispatch([this] () mutable {
String fileContents = readContentsOfFile(tempFilePath());
@@ -192,7 +191,7 @@
auto testQueue = WorkQueue::create("Test Work Queue");
- auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
+ auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
ASSERT(!RunLoop::isMain());
switch (type) {
case FileMonitor::FileChangeType::Modification:
@@ -203,7 +202,6 @@
break;
}
});
- monitor->startMonitoring();
testQueue->dispatch([this] () mutable {
String fileContents = readContentsOfFile(tempFilePath());
@@ -253,7 +251,7 @@
auto testQueue = WorkQueue::create("Test Work Queue");
- auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
+ auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
ASSERT(!RunLoop::isMain());
switch (type) {
case FileMonitor::FileChangeType::Modification:
@@ -264,7 +262,6 @@
break;
}
});
- monitor->startMonitoring();
testQueue->dispatch([this] () mutable {
StringBuilder command;
@@ -293,7 +290,7 @@
auto testQueue = WorkQueue::create("Test Work Queue");
- auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
+ auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
ASSERT(!RunLoop::isMain());
switch (type) {
case FileMonitor::FileChangeType::Modification:
@@ -304,7 +301,6 @@
break;
}
});
- monitor->startMonitoring();
testQueue->dispatch([this] () mutable {
String fileContents = readContentsOfFile(tempFilePath());
@@ -351,7 +347,7 @@
auto testQueue = WorkQueue::create("Test Work Queue");
- auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
+ auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
ASSERT(!RunLoop::isMain());
switch (type) {
case FileMonitor::FileChangeType::Modification:
@@ -362,7 +358,6 @@
break;
}
});
- monitor->startMonitoring();
testQueue->dispatch([this] () mutable {
StringBuilder command;
