Changeset 219219 in webkit


Ignore:
Timestamp:
Jul 6, 2017 2:45:04 PM (7 years ago)
Author:
Chris Dumez
Message:

FileMonitor should not be ref counted
​https://bugs.webkit.org/show_bug.cgi?id=174166

Reviewed by Brent Fulgham.

Source/WebCore:

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.

Source/WebKit2:

Update code using FileMonitor to reflect API change.

  • UIProcess/WebResourceLoadStatisticsStore.cpp:

(WebKit::WebResourceLoadStatisticsStore::startMonitoringStatisticsStorage):
(WebKit::WebResourceLoadStatisticsStore::stopMonitoringStatisticsStorage):

  • UIProcess/WebResourceLoadStatisticsStore.h:

Tools:

Update the API tests to reflect the API change.

  • TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:

(TestWebKitAPI::TEST_F):

Location:
trunk
Files:
9 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r219218 r219219  
     12017-07-06  Chris Dumez  <cdumez@apple.com>
     2
     3        FileMonitor should not be ref counted
     4        https://bugs.webkit.org/show_bug.cgi?id=174166
     5
     6        Reviewed by Brent Fulgham.
     7
     8        Update FileMonitor to no longer be refcounted. It was previously easy to leak it
     9        because the object would ref itself in various lambdas. The client would have to
     10        explicitely call FileMonitor::stopMonitoring() which was fragile.
     11
     12        This patch also simplifies the code and API a bit since no longer actually
     13        requires startMonitoring() / stopMonitoring() API.
     14
     15        No new tests, covered by API tests.
     16
     17        * platform/FileMonitor.cpp:
     18        (WebCore::FileMonitor::FileMonitor):
     19        (WebCore::FileMonitor::~FileMonitor):
     20        (WebCore::FileMonitor::create): Deleted.
     21        (WebCore::FileMonitor::startMonitoring): Deleted.
     22        (WebCore::FileMonitor::stopMonitoring): Deleted.
     23        * platform/FileMonitor.h:
     24        * platform/cocoa/FileMonitorCocoa.mm:
     25        (WebCore::FileMonitor::FileMonitor):
     26        (WebCore::FileMonitor::~FileMonitor):
     27        (WebCore::FileMonitor::startMonitoring): Deleted.
     28        (WebCore::FileMonitor::stopMonitoring): Deleted.
     29
    1302017-07-06  Matt Rajca  <mrajca@apple.com>
    231

  • trunk/Source/WebCore/platform/FileMonitor.cpp

    r218901 r219219  
    2929namespace WebCore {
    3030
    31 Ref<FileMonitor> FileMonitor::create(const String& path, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler)
    32 {
    33     return adoptRef(*new FileMonitor(path, WTFMove(handlerQueue), WTFMove(modificationHandler)));
    34 }
    35    
    36 FileMonitor::FileMonitor(const String& path, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler)
    37     : m_path(path)
    38     , m_modificationHandler(WTFMove(modificationHandler))
    39     , m_handlerQueue(WTFMove(handlerQueue))
     31#if !PLATFORM(COCOA)
     32
     33FileMonitor::FileMonitor(const String&, Ref<WorkQueue>&&, WTF::Function<void(FileChangeType)>&&)
    4034{
    4135}
    42    
     36
    4337FileMonitor::~FileMonitor()
    4438{
    45     stopMonitoring();
    46 }
    47    
    48 #if !PLATFORM(COCOA)
    49 void FileMonitor::startMonitoring()
    50 {
    51     // Do Nothing
    52 }
    53    
    54 void FileMonitor::stopMonitoring()
    55 {
    56     // Do Nothing
    5739}
    5840

  • trunk/Source/WebCore/platform/FileMonitor.h

    r218901 r219219  
    2727
    2828#include <wtf/Function.h>
    29 #include <wtf/ThreadSafeRefCounted.h>
    3029#include <wtf/WorkQueue.h>
    3130#include <wtf/text/WTFString.h>
    … …  
    3837namespace WebCore {
    3938
    40 class FileMonitor : public ThreadSafeRefCounted<FileMonitor> {
     39class FileMonitor {
    4140public:
    4241    enum class FileChangeType {
    … …  
    4544    };
    4645
    47     WEBCORE_EXPORT static Ref<FileMonitor> create(const String&, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler);
     46    WEBCORE_EXPORT FileMonitor(const String&, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler);
    4847    WEBCORE_EXPORT ~FileMonitor();
    4948
    50     WEBCORE_EXPORT void startMonitoring();
    51     WEBCORE_EXPORT void stopMonitoring();
    52 
    5349private:
    54     FileMonitor(const String&, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler);
    55 
    56     String m_path;
    57     WTF::Function<void(FileChangeType)> m_modificationHandler;
    58     Ref<WTF::WorkQueue> m_handlerQueue;
    59 
    6050#if USE(COCOA_EVENT_LOOP)
    6151    DispatchPtr<dispatch_source_t> m_platformMonitor;

  • trunk/Source/WebCore/platform/cocoa/FileMonitorCocoa.mm

    r219144 r219219  
    2929#import "FileSystem.h"
    3030#import "Logging.h"
    31 #import <dispatch/dispatch.h>
    32 #import <wtf/DispatchPtr.h>
     31#import <wtf/BlockPtr.h>
    3332
    3433namespace WebCore {
    … …  
    3736constexpr unsigned fileUnavailableMask = DISPATCH_VNODE_DELETE | DISPATCH_VNODE_RENAME | DISPATCH_VNODE_REVOKE;
    3837
    39 void FileMonitor::startMonitoring()
     38FileMonitor::FileMonitor(const String& path, Ref<WorkQueue>&& handlerQueue, WTF::Function<void(FileChangeType)>&& modificationHandler)
    4039{
    41     if (m_platformMonitor)
     40    if (path.isEmpty())
    4241        return;
    4342
    44     if (m_path.isEmpty())
     43    if (!modificationHandler)
    4544        return;
    4645
    47     if (!m_modificationHandler)
    48         return;
    49 
    50     auto handle = openFile(m_path, OpenForEventsOnly);
     46    auto handle = openFile(path, OpenForEventsOnly);
    5147    if (handle == invalidPlatformFileHandle) {
    52         RELEASE_LOG_ERROR(ResourceLoadStatistics, "Failed to open statistics file for monitoring: %s", m_path.utf8().data());
     48        RELEASE_LOG_ERROR(ResourceLoadStatistics, "Failed to open statistics file for monitoring: %s", path.utf8().data());
    5349        return;
    5450    }
    5551
    56     m_platformMonitor = adoptDispatch(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle, monitorMask, m_handlerQueue->dispatchQueue()));
     52    // The source (platformMonitor) retains the dispatch queue.
     53    m_platformMonitor = adoptDispatch(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle, monitorMask, handlerQueue->dispatchQueue()));
    5754
    5855    LOG(ResourceLoadStatistics, "Creating monitor %p", m_platformMonitor.get());
    5956
    60     dispatch_source_set_event_handler(m_platformMonitor.get(), [this, protectedThis = makeRefPtr(this), fileMonitor = m_platformMonitor.get()] () mutable {
     57    dispatch_source_set_event_handler(m_platformMonitor.get(), BlockPtr<void()>::fromCallable([modificationHandler = WTFMove(modificationHandler), fileMonitor = m_platformMonitor] {
    6158        // If this is getting called after the monitor was cancelled, just drop the notification.
    62         if (dispatch_source_testcancel(fileMonitor))
     59        if (dispatch_source_testcancel(fileMonitor.get()))
    6360            return;
    6461
    65         unsigned flag = dispatch_source_get_data(fileMonitor);
    66         LOG(ResourceLoadStatistics, "File event %#X for monitor %p", flag, fileMonitor);
     62        unsigned flag = dispatch_source_get_data(fileMonitor.get());
     63        LOG(ResourceLoadStatistics, "File event %#X for monitor %p", flag, fileMonitor.get());
    6764        if (flag & fileUnavailableMask) {
    68             m_modificationHandler(FileChangeType::Removal);
    69             dispatch_source_cancel(fileMonitor);
     65            modificationHandler(FileChangeType::Removal);
     66            dispatch_source_cancel(fileMonitor.get());
    7067        } else {
    7168            ASSERT(flag & DISPATCH_VNODE_WRITE);
    72             m_modificationHandler(FileChangeType::Modification);
     69            modificationHandler(FileChangeType::Modification);
    7370        }
    74     });
     71    }).get());
    7572   
    76     dispatch_source_set_cancel_handler(m_platformMonitor.get(), [fileMonitor = m_platformMonitor.get()] {
    77         auto handle = static_cast<PlatformFileHandle>(dispatch_source_get_handle(fileMonitor));
     73    dispatch_source_set_cancel_handler(m_platformMonitor.get(), [handle] () mutable {
    7874        closeFile(handle);
    7975    });
    … …  
    8177    dispatch_resume(m_platformMonitor.get());
    8278}
    83    
    84 void FileMonitor::stopMonitoring()
     79
     80FileMonitor::~FileMonitor()
    8581{
    8682    if (!m_platformMonitor)
    … …  
    9086
    9187    dispatch_source_cancel(m_platformMonitor.get());
    92     m_platformMonitor = nullptr;
    9388}
    9489

  • trunk/Source/WebKit2/ChangeLog

    r219218 r219219  
     12017-07-06  Chris Dumez  <cdumez@apple.com>
     2
     3        FileMonitor should not be ref counted
     4        https://bugs.webkit.org/show_bug.cgi?id=174166
     5
     6        Reviewed by Brent Fulgham.
     7
     8        Update code using FileMonitor to reflect API change.
     9
     10        * UIProcess/WebResourceLoadStatisticsStore.cpp:
     11        (WebKit::WebResourceLoadStatisticsStore::startMonitoringStatisticsStorage):
     12        (WebKit::WebResourceLoadStatisticsStore::stopMonitoringStatisticsStorage):
     13        * UIProcess/WebResourceLoadStatisticsStore.h:
     14
    1152017-07-06  Matt Rajca  <mrajca@apple.com>
    216

  • trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp

    r219211 r219219  
    435435        return;
    436436   
    437     m_statisticsStorageMonitor = FileMonitor::create(resourceLogPath, m_statisticsQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
     437    m_statisticsStorageMonitor = std::make_unique<FileMonitor>(resourceLogPath, m_statisticsQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
    438438        ASSERT(!RunLoop::isMain());
    439439        switch (type) {
    … …  
    447447        }
    448448    });
    449 
    450     m_statisticsStorageMonitor->startMonitoring();
    451449}
    452450
    … …  
    454452{
    455453    ASSERT(!RunLoop::isMain());
    456     if (!m_statisticsStorageMonitor)
    457         return;
    458 
    459     // FIXME(174166): The FileMonitor captures itself, incrementing its refcount. Manually stopping the monitor shuts down the lambda holding the extra
    460     // reference, so the object will be cleaned up properly.
    461     m_statisticsStorageMonitor->stopMonitoring();
    462454    m_statisticsStorageMonitor = nullptr;
    463455}

  • trunk/Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h

    r219211 r219219  
    146146#endif
    147147    Ref<WTF::WorkQueue> m_statisticsQueue;
    148     RefPtr<WebCore::FileMonitor> m_statisticsStorageMonitor;
     148    std::unique_ptr<WebCore::FileMonitor> m_statisticsStorageMonitor;
    149149    const String m_statisticsStoragePath;
    150150    WTF::WallTime m_lastStatisticsFileSyncTime;

  • trunk/Tools/ChangeLog

    r219202 r219219  
     12017-07-06  Chris Dumez  <cdumez@apple.com>
     2
     3        FileMonitor should not be ref counted
     4        https://bugs.webkit.org/show_bug.cgi?id=174166
     5
     6        Reviewed by Brent Fulgham.
     7
     8        Update the API tests to reflect the API change.
     9
     10        * TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:
     11        (TestWebKitAPI::TEST_F):
     12
    1132017-07-06  Commit Queue  <commit-queue@webkit.org>
    214

  • trunk/Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp

    r218906 r219219  
    150150    auto testQueue = WorkQueue::create("Test Work Queue");
    151151
    152     auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
     152    auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
    153153        ASSERT(!RunLoop::isMain());
    154154        switch (type) {
    … …  
    161161        }
    162162    });
    163     monitor->startMonitoring();
    164163
    165164    testQueue->dispatch([this] () mutable {
    … …  
    193192    auto testQueue = WorkQueue::create("Test Work Queue");
    194193
    195     auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
     194    auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
    196195        ASSERT(!RunLoop::isMain());
    197196        switch (type) {
    … …  
    204203        }
    205204    });
    206     monitor->startMonitoring();
    207205   
    208206    testQueue->dispatch([this] () mutable {
    … …  
    254252    auto testQueue = WorkQueue::create("Test Work Queue");
    255253
    256     auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
     254    auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
    257255        ASSERT(!RunLoop::isMain());
    258256        switch (type) {
    … …  
    265263        }
    266264    });
    267     monitor->startMonitoring();
    268265
    269266    testQueue->dispatch([this] () mutable {
    … …  
    294291    auto testQueue = WorkQueue::create("Test Work Queue");
    295292
    296     auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
     293    auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
    297294        ASSERT(!RunLoop::isMain());
    298295        switch (type) {
    … …  
    305302        }
    306303    });
    307     monitor->startMonitoring();
    308304
    309305    testQueue->dispatch([this] () mutable {
    … …  
    352348    auto testQueue = WorkQueue::create("Test Work Queue");
    353349
    354     auto monitor = WebCore::FileMonitor::create(tempFilePath(), testQueue.copyRef(), [this] (WebCore::FileMonitor::FileChangeType type) {
     350    auto monitor = std::make_unique<FileMonitor>(tempFilePath(), testQueue.copyRef(), [this] (FileMonitor::FileChangeType type) {
    355351        ASSERT(!RunLoop::isMain());
    356352        switch (type) {
    … …  
    363359        }
    364360    });
    365     monitor->startMonitoring();
    366361
    367362    testQueue->dispatch([this] () mutable {
Note: See TracChangeset for help on using the changeset viewer.