RESOLVED FIXED 173800
Teach ResourceLoadStatistics to recognize changes in the file system
https://bugs.webkit.org/show_bug.cgi?id=173800
Summary Teach ResourceLoadStatistics to recognize changes in the file system
Brent Fulgham
Reported 2017-06-23 17:21:22 PDT
Revise the handling of the local statistics store so that it recognizes changes to the data store while the process is running, and pulls in the updated file. Help avoid race conditions by using file locks to prevent two processes from interacting with the file at the same time.
Attachments
Patch (18.66 KB, patch)
2017-06-23 18:02 PDT, Brent Fulgham
no flags
Patch (18.83 KB, patch)
2017-06-24 16:09 PDT, Brent Fulgham
no flags
Patch v3 - use dispatch routines (20.84 KB, patch)
2017-06-25 10:29 PDT, Brent Fulgham
no flags
Patch v4: Correct iOS build. (20.71 KB, patch)
2017-06-25 13:35 PDT, Brent Fulgham
no flags
Patch v5: Correct file deletion behavior. (20.66 KB, patch)
2017-06-26 09:50 PDT, Brent Fulgham
no flags
Patch (46.26 KB, patch)
2017-06-27 10:43 PDT, Brent Fulgham
no flags
Patch (46.27 KB, patch)
2017-06-27 10:46 PDT, Brent Fulgham
no flags
Patch (47.20 KB, patch)
2017-06-27 14:05 PDT, Brent Fulgham
no flags
Complete Patch (with initial test case) (47.16 KB, patch)
2017-06-27 14:20 PDT, Brent Fulgham
no flags
Patch (Rebaselined). (47.36 KB, patch)
2017-06-27 14:32 PDT, Brent Fulgham
no flags
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.18 MB, application/zip)
2017-06-27 16:29 PDT, Build Bot
no flags
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.44 MB, application/zip)
2017-06-27 16:35 PDT, Build Bot
no flags
Patch (49.00 KB, patch)
2017-06-27 17:53 PDT, Brent Fulgham
no flags
Patch (Fix GTK/WPE Builds) (49.53 KB, patch)
2017-06-27 17:58 PDT, Brent Fulgham
no flags
Patch (50.26 KB, patch)
2017-06-27 18:30 PDT, Brent Fulgham
no flags
Patch (58.23 KB, patch)
2017-06-28 13:45 PDT, Brent Fulgham
cdumez: review+
cdumez: commit-queue-
Brent Fulgham
Comment 1 2017-06-23 17:21:46 PDT
Brent Fulgham
Comment 2 2017-06-23 18:02:13 PDT
Brent Fulgham
Comment 3 2017-06-23 18:02:48 PDT
I'm working on a separate test program for this in TestWebKitAPI. Would like to do that as a separate patch, since it will end up being fairly large.
Brent Fulgham
Comment 4 2017-06-24 16:09:35 PDT
Chris Dumez
Comment 5 2017-06-24 16:39:01 PDT
Comment on attachment 313791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313791&action=review I am not the right person to review this but here are some drive-by comments. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:296 > +{ Can we add an assertion to make sure this is always called from the thread you expect it to be called on? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:310 > +{ Can we add an assertion to make sure this is always called from the thread you expect it to be called on? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:349 > String resourceLog = persistentStoragePath(label); Could we add an assertion to make sure we are not on the main thread since this does FS I/O? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:377 > + RefPtr<SharedBuffer> rawData = SharedBuffer::create(WTFMove(buffer)); You already have all the data in a Vector, is there any reason why we need to construct a SharedBuffer rather than just call KeyedDecoder::decoder() with a pointer to the Vector buffer? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:387 > + String resourceLogPath = persistentStoragePath("full_browsing_session"); ASCIILiteral("full_browsing_session"); > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:61 > + String path = paths[i]; You probably want String::fromUTF8() here? > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:81 > + String resourceLogPath = persistentStoragePath("full_browsing_session"); ASCIILiteral() > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:85 > + CString fsRep = fileSystemRepresentation(resourceLogPath); auto. Also fsRep is not a good name. > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:88 > + RetainPtr<CFStringRef> fileToWatch = adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, fsRep.data(), fsRep.length())); auto > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:92 > + RetainPtr<CFArrayRef> pathsToWatch = adoptCF(CFArrayCreate(kCFAllocatorDefault, (const void**)&fileToWatchRef, 1, &kCFTypeArrayCallBacks)); auto > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:110 > + WTFLogAlways("Unable to begin file monitoring for %s.", fsRep.data()); Seems like all your WTFLogAlways() calls should really be LOG_ALWAYS_ERROR().
Sam Weinig
Comment 6 2017-06-24 17:16:51 PDT
Comment on attachment 313791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313791&action=review > Source/WebKit2/ChangeLog:10 > + Update the ResourceLoadStatistics logic to be aware that the statistics data > + file might change underneath it, and to take appropriate action when it does. Why is this being done? What problem is it solving? > Source/WebKit2/config.h:145 > +#if PLATFORM(COCOA) > +#define USE_FILE_LOCK 1 > +#endif This seems like something that should go into Platform.h > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:304 > +#if USE(FILE_LOCK) > + bool locked = lockFile(handle, WebCore::LockExclusive); > + ASSERT_UNUSED(locked, locked); > +#endif Is this code valid if you don't have file locks? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:120 > +#if PLATFORM(COCOA) > + RetainPtr<FSEventStreamRef> m_statisticsStorageMonitoringStream; > +#endif Seems inappropriate to use a platform specific type here. This should be abstracted as we do with other platform specific concepts.
Brent Fulgham
Comment 7 2017-06-24 22:06:31 PDT
Comment on attachment 313791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313791&action=review >> Source/WebKit2/config.h:145 >> +#endif > > This seems like something that should go into Platform.h Sounds good. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:296 >> +{ > > Can we add an assertion to make sure this is always called from the thread you expect it to be called on? Will do. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:304 >> +#endif > > Is this code valid if you don't have file locks? For Cocoa purposes, we want a locked file. Other platforms may not care. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:310 >> +{ > > Can we add an assertion to make sure this is always called from the thread you expect it to be called on? Will do. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:349 >> String resourceLog = persistentStoragePath(label); > > Could we add an assertion to make sure we are not on the main thread since this does FS I/O? Yes. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:377 >> + RefPtr<SharedBuffer> rawData = SharedBuffer::create(WTFMove(buffer)); > > You already have all the data in a Vector, is there any reason why we need to construct a SharedBuffer rather than just call KeyedDecoder::decoder() with a pointer to the Vector buffer? Ugh. Yes, I am an idiot! >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:387 >> + String resourceLogPath = persistentStoragePath("full_browsing_session"); > > ASCIILiteral("full_browsing_session"); Will do. >> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:61 >> + String path = paths[i]; > > You probably want String::fromUTF8() here? Good idea. >> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:81 >> + String resourceLogPath = persistentStoragePath("full_browsing_session"); > > ASCIILiteral() Will do. >> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:85 >> + CString fsRep = fileSystemRepresentation(resourceLogPath); > > auto. Also fsRep is not a good name. fsRep -> fileSystemPath >> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:88 >> + RetainPtr<CFStringRef> fileToWatch = adoptCF(CFStringCreateWithCString(kCFAllocatorDefault, fsRep.data(), fsRep.length())); > > auto OK! >> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:92 >> + RetainPtr<CFArrayRef> pathsToWatch = adoptCF(CFArrayCreate(kCFAllocatorDefault, (const void**)&fileToWatchRef, 1, &kCFTypeArrayCallBacks)); > > auto OK! >> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:110 >> + WTFLogAlways("Unable to begin file monitoring for %s.", fsRep.data()); > > Seems like all your WTFLogAlways() calls should really be LOG_ALWAYS_ERROR(). LOG_ERROR?
Chris Dumez
Comment 8 2017-06-24 22:27:48 PDT
Comment on attachment 313791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313791&action=review >>> Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:110 >>> + WTFLogAlways("Unable to begin file monitoring for %s.", fsRep.data()); >> >> Seems like all your WTFLogAlways() calls should really be LOG_ALWAYS_ERROR(). > > LOG_ERROR? No RELEASE_LOG_ERROR() so it gets printed in release builds, like your current code. But will show up as en error in Console.
Brent Fulgham
Comment 9 2017-06-25 10:29:16 PDT
Created attachment 313809 [details] Patch v3 - use dispatch routines
Brent Fulgham
Comment 10 2017-06-25 10:30:08 PDT
Comment on attachment 313809 [details] Patch v3 - use dispatch routines CoreServices isn't really available on iOS. Instead, use dispatch sources to monitor file activity.
Brent Fulgham
Comment 11 2017-06-25 11:55:10 PDT
Drat. I left the stupid CoreServices include in the file!
Brent Fulgham
Comment 12 2017-06-25 13:35:38 PDT
Created attachment 313813 [details] Patch v4: Correct iOS build.
Brent Fulgham
Comment 13 2017-06-26 09:50:21 PDT
Created attachment 313847 [details] Patch v5: Correct file deletion behavior.
Brent Fulgham
Comment 14 2017-06-26 09:53:01 PDT
Comment on attachment 313847 [details] Patch v5: Correct file deletion behavior. During testing I discovered the "respond to deleted file" case would prematurely destroy the dispatch element, causing a nullptr exception. This version correct this.
Brent Fulgham
Comment 15 2017-06-26 12:06:15 PDT
Note: Test infrastructure to support automated testing of these changes are being done under Bug 173845.
Chris Dumez
Comment 16 2017-06-26 12:23:32 PDT
Comment on attachment 313847 [details] Patch v5: Correct file deletion behavior. View in context: https://bugs.webkit.org/attachment.cgi?id=313847&action=review > Source/WTF/ChangeLog:3 > + Teach ResourceLoadStatistics to recognize changes in the file system I am still not clear about the why? Either from this bug or the radar. > Source/WTF/wtf/Platform.h:548 > +#define USE_FILE_LOCK 1 Do we still need it in WebCore/config.h if it is here?
Sam Weinig
Comment 17 2017-06-26 13:56:54 PDT
Comment on attachment 313847 [details] Patch v5: Correct file deletion behavior. View in context: https://bugs.webkit.org/attachment.cgi?id=313847&action=review > Source/WebKit2/ChangeLog:10 > + Update the ResourceLoadStatistics logic to be aware that the statistics data > + file might change underneath it, and to take appropriate action when it does. As mentioned in my previous review: "Why is this being done? What problem is it solving?" > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:320 > +#if USE(FILE_LOCK) > + bool locked = lockFile(handle, WebCore::LockExclusive); > + ASSERT_UNUSED(locked, locked); > +#endif I still don't understand why one platform would want locks and another would not. Can you go into detail about why that is? > Source/WebKit2/UIProcess/Cocoa/WebResourceLoadStatisticsStoreCocoa.mm:74 > + m_statisticsStorageMonitor = adoptOSObject(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle, monitorMask, dispatch_get_main_queue())); > + > + dispatch_set_context(m_statisticsStorageMonitor.get(), this); > + It looks like you didn't take my review feedback on creating a proper abstraction for this. Is there some objection you have to it?
Chris Dumez
Comment 18 2017-06-26 14:01:16 PDT
Comment on attachment 313847 [details] Patch v5: Correct file deletion behavior. View in context: https://bugs.webkit.org/attachment.cgi?id=313847&action=review >> Source/WebKit2/ChangeLog:10 >> + file might change underneath it, and to take appropriate action when it does. > > As mentioned in my previous review: > > "Why is this being done? What problem is it solving?" There is now clarification about this on the radar.
Sam Weinig
Comment 19 2017-06-26 14:10:01 PDT
(In reply to Chris Dumez from comment #18) > Comment on attachment 313847 [details] > Patch v5: Correct file deletion behavior. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313847&action=review > > >> Source/WebKit2/ChangeLog:10 > >> + file might change underneath it, and to take appropriate action when it does. > > > > As mentioned in my previous review: > > > > "Why is this being done? What problem is it solving?" > > There is now clarification about this on the radar. :\
Brent Fulgham
Comment 20 2017-06-26 14:28:29 PDT
(In reply to Sam Weinig from comment #17) > Comment on attachment 313847 [details] > Patch v5: Correct file deletion behavior. > > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:320 > > +#if USE(FILE_LOCK) > > + bool locked = lockFile(handle, WebCore::LockExclusive); > > + ASSERT_UNUSED(locked, locked); > > +#endif > > I still don't understand why one platform would want locks and another would > not. Can you go into detail about why that is? Only one platform is currently using this. That platform does have FILE_LOCK. If a platform does not support file locks, but also does not expect this data file to be touched by more than one process, then there is no need to be concerned about this case. The fact that we have the USE(FILE_LOCK) macro implies such platform exist, though I don't know what platform that would be.
Brent Fulgham
Comment 21 2017-06-26 14:29:17 PDT
Comment on attachment 313847 [details] Patch v5: Correct file deletion behavior. View in context: https://bugs.webkit.org/attachment.cgi?id=313847&action=review >> Source/WTF/wtf/Platform.h:548 >> +#define USE_FILE_LOCK 1 > > Do we still need it in WebCore/config.h if it is here? No -- it should be removed from WebCore/config.h.
Chris Dumez
Comment 22 2017-06-26 14:38:43 PDT
Comment on attachment 313847 [details] Patch v5: Correct file deletion behavior. View in context: https://bugs.webkit.org/attachment.cgi?id=313847&action=review > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:69 > + stopMonitoringStatisticsStorage(); Could we al least move the monitor to its own class so it would stop monitoring when destroyed? It looks unfortunate/fragile to have to call stopMonitoringStatisticsStorage() in this destructor.
Brent Fulgham
Comment 23 2017-06-26 15:16:02 PDT
(In reply to Chris Dumez from comment #18) > Comment on attachment 313847 [details] > Patch v5: Correct file deletion behavior. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313847&action=review > > >> Source/WebKit2/ChangeLog:10 > >> + file might change underneath it, and to take appropriate action when it does. > > > > As mentioned in my previous review: > > > > "Why is this being done? What problem is it solving?" > > There is now clarification about this on the radar. That said, I can add a Changelog comment, "We want to support the case where multiple UI processes choose to share the same statistics file”.
Sam Weinig
Comment 24 2017-06-26 21:13:33 PDT
(In reply to Brent Fulgham from comment #20) > (In reply to Sam Weinig from comment #17) > > Comment on attachment 313847 [details] > > Patch v5: Correct file deletion behavior. > > > > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:320 > > > +#if USE(FILE_LOCK) > > > + bool locked = lockFile(handle, WebCore::LockExclusive); > > > + ASSERT_UNUSED(locked, locked); > > > +#endif > > > > I still don't understand why one platform would want locks and another would > > not. Can you go into detail about why that is? > > Only one platform is currently using this. That platform does have FILE_LOCK. > > If a platform does not support file locks, but also does not expect this > data file to be touched by more than one process, then there is no need to > be concerned about this case. > > The fact that we have the USE(FILE_LOCK) macro implies such platform exist, > though I don't know what platform that would be. I think a specific conditional would be more appropriate here then. Something like ENABLE(MULTIPROCESS_ACCESS_TO_STATISTICS_STORE).
Brent Fulgham
Comment 25 2017-06-27 10:43:49 PDT
Brent Fulgham
Comment 26 2017-06-27 10:46:26 PDT
Chris Dumez
Comment 27 2017-06-27 11:30:26 PDT
Comment on attachment 313927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313927&action=review Very quick pass. > Source/WebCore/platform/FileMonitor.h:39 > +class FileMonitor : public RefCounted<FileMonitor> { Does this really need to be refcounted? > Source/WebCore/platform/FileMonitor.h:40 > + WTF_MAKE_NONCOPYABLE(FileMonitor); Unnecessary if RefCounted > Source/WebCore/platform/FileMonitor.h:41 > + WTF_MAKE_FAST_ALLOCATED; Unnecessary if RefCounted > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:157 > + RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] () { Isn't this racy? We stop monitoring on the main thread but we start writing right away on the background thread. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:374 > + RELEASE_LOG_ERROR(ResourceLoadStatistics, "WebResourceLoadStatisticsStore: We only wrote %d out of %d bytes to disk", static_cast<unsigned>(writtenBytes), rawData->size()); Why is this using %d for unsigned values? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:384 > + if (!deleteFile(resourceLogPath)) What if we are monitoring? won't this notify us?
Chris Dumez
Comment 28 2017-06-27 11:36:57 PDT
Comment on attachment 313927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313927&action=review > Source/WebCore/platform/FileMonitor.cpp:31 > +Ref<FileMonitor> FileMonitor::create(const String& path, WTF::Function<void()>&& modificationHandler, WTF::Function<void()>&& deletionHandler) This may look better (and less error prone due to parameter swap) to take only one lambda parameter and pass a ChangeType parameter to that lambda. E.g. enum class FileChangeType { Modificication, Removal }.
Brent Fulgham
Comment 29 2017-06-27 11:49:12 PDT
Comment on attachment 313927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313927&action=review >> Source/WebCore/platform/FileMonitor.cpp:31 >> +Ref<FileMonitor> FileMonitor::create(const String& path, WTF::Function<void()>&& modificationHandler, WTF::Function<void()>&& deletionHandler) > > This may look better (and less error prone due to parameter swap) to take only one lambda parameter and pass a ChangeType parameter to that lambda. E.g. enum class FileChangeType { Modificication, Removal }. OK! >> Source/WebCore/platform/FileMonitor.h:39 >> +class FileMonitor : public RefCounted<FileMonitor> { > > Does this really need to be refcounted? No -- I should use a unique_ptr here. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:157 >> + RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] () { > > Isn't this racy? We stop monitoring on the main thread but we start writing right away on the background thread. Would it be better to chain the dispatch of the write operation from within the main loop (and the subsequent re-activation of monitoring after? Probably. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:374 >> + RELEASE_LOG_ERROR(ResourceLoadStatistics, "WebResourceLoadStatisticsStore: We only wrote %d out of %d bytes to disk", static_cast<unsigned>(writtenBytes), rawData->size()); > > Why is this using %d for unsigned values? Oh! I'll fix. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:384 >> + if (!deleteFile(resourceLogPath)) > > What if we are monitoring? won't this notify us? Good point. We should stop monitoring when we delete the file, as we'll need to create a new monitor object for the changed file descriptor.
Brent Fulgham
Comment 30 2017-06-27 14:05:40 PDT
Brent Fulgham
Comment 31 2017-06-27 14:20:10 PDT
Created attachment 313942 [details] Complete Patch (with initial test case)
Brent Fulgham
Comment 32 2017-06-27 14:21:23 PDT
Comment on attachment 313942 [details] Complete Patch (with initial test case) This patch incorporates all existing comments, and includes basic testing of the FileMonitor class.
Brent Fulgham
Comment 33 2017-06-27 14:32:25 PDT
Created attachment 313945 [details] Patch (Rebaselined).
Antti Koivisto
Comment 34 2017-06-27 15:10:54 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). View in context: https://bugs.webkit.org/attachment.cgi?id=313945&action=review > Source/WebCore/platform/FileMonitor.h:62 > + dispatch_source_t m_platformMonitor { nullptr }; > + dispatch_queue_t m_dispatchQueue { nullptr }; It would be nicer to use smart pointers. There is DispatchPtr in NetworkCacheData.h. It could be moved to WTF. It is also possible that OSObjectPtr works with dispatch types these days (it had some problems on older OS). > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:69 > + if (protectedThis->m_modificationHandler) > + protectedThis->m_modificationHandler(FileMonitor::Removal); > + dispatch_source_cancel(protectedThis->m_platformMonitor); > + } else { > + if (protectedThis->m_modificationHandler) > + protectedThis->m_modificationHandler(FileMonitor::Modification); If you captured 'this' too you could avoid repeating protectedThis-> everywhere.
Brent Fulgham
Comment 35 2017-06-27 15:16:32 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). View in context: https://bugs.webkit.org/attachment.cgi?id=313945&action=review >> Source/WebCore/platform/FileMonitor.h:62 >> + dispatch_queue_t m_dispatchQueue { nullptr }; > > It would be nicer to use smart pointers. There is DispatchPtr in NetworkCacheData.h. It could be moved to WTF. It is also possible that OSObjectPtr works with dispatch types these days (it had some problems on older OS). Cool! I did try OSObjectPtr first, but it was not happy with dispatch types. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:69 >> + protectedThis->m_modificationHandler(FileMonitor::Modification); > > If you captured 'this' too you could avoid repeating protectedThis-> everywhere. OK!
Chris Dumez
Comment 36 2017-06-27 15:21:54 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). View in context: https://bugs.webkit.org/attachment.cgi?id=313945&action=review > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:56 > + RefPtr<FileMonitor> protectedThis(this); Why is this needed since you cancel the source in the destructor of FileMonitor?
Brent Fulgham
Comment 37 2017-06-27 15:27:10 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). View in context: https://bugs.webkit.org/attachment.cgi?id=313945&action=review >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:56 >> + RefPtr<FileMonitor> protectedThis(this); > > Why is this needed since you cancel the source in the destructor of FileMonitor? dispatch_cancel doesn't kill any enqueued tasks. So cancelling there, and even destroying the dispatch_source doesn't stop the event handler from receiving more events.
Chris Dumez
Comment 38 2017-06-27 15:49:56 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). View in context: https://bugs.webkit.org/attachment.cgi?id=313945&action=review > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:73 > + dispatch_source_set_cancel_handler(m_platformMonitor, [protectedThis] { If we don't need the cleanup function, we can probably just capture the handle here? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:87 > + dispatch_source_cancel(m_platformMonitor); Cannot we dispatch_release(m_platformMonitor) and dispatch_release(m_dispatchQueue) right after this? Do we really need this cleanup function?
Chris Dumez
Comment 39 2017-06-27 15:58:26 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). View in context: https://bugs.webkit.org/attachment.cgi?id=313945&action=review > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:65 > + protectedThis->m_modificationHandler(FileMonitor::Removal); Isn't this handler called on the File monitoring queue? If so, doesn't this means we're calling the modification handler on a background thread? Is it safe? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:414 > + m_statisticsStorageMonitor = FileMonitor::create(resourceLogPath, [this] (FileMonitor::FileChangeType type) { I am still trying to fully understand the patch but would it make sense to have the FileMonitor constructor take a WorkQueue in parameter so that we can pass m_statisticsQueue ? This way, our handler would get called on the right thread.
Build Bot
Comment 40 2017-06-27 16:29:50 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). Attachment 313945 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4009327 New failing tests: http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-sub-frame-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-mixed-statistics.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-sub-frame-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/telemetry-generation.html http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-mixed-statistics.html http/tests/loading/resourceLoadStatistics/clear-in-memory-and-persistent-store.html http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-subresource-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-subresource-unique-redirects-to.html http/tests/loading/resourceLoadStatistics/non-prevalent-resource-without-user-interaction.html http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-subresource-unique-redirects-to.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-subresource-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/prevalent-resource-with-user-interaction.html
Build Bot
Comment 41 2017-06-27 16:29:52 PDT
Created attachment 313957 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 42 2017-06-27 16:35:07 PDT
Comment on attachment 313945 [details] Patch (Rebaselined). Attachment 313945 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4009281 New failing tests: http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-sub-frame-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-mixed-statistics.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-sub-frame-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/telemetry-generation.html http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-mixed-statistics.html http/tests/loading/resourceLoadStatistics/clear-in-memory-and-persistent-store.html http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-subresource-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-subresource-unique-redirects-to.html http/tests/loading/resourceLoadStatistics/non-prevalent-resource-without-user-interaction.html http/tests/loading/resourceLoadStatistics/classify-as-non-prevalent-based-on-subresource-unique-redirects-to.html http/tests/loading/resourceLoadStatistics/classify-as-prevalent-based-on-subresource-under-top-frame-origins.html http/tests/loading/resourceLoadStatistics/prevalent-resource-with-user-interaction.html
Build Bot
Comment 43 2017-06-27 16:35:09 PDT
Created attachment 313958 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Brent Fulgham
Comment 44 2017-06-27 17:53:15 PDT
Brent Fulgham
Comment 45 2017-06-27 17:58:22 PDT
Created attachment 313968 [details] Patch (Fix GTK/WPE Builds)
Brent Fulgham
Comment 46 2017-06-27 18:00:12 PDT
(In reply to Chris Dumez from comment #39) > > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:414 > > + m_statisticsStorageMonitor = FileMonitor::create(resourceLogPath, [this] (FileMonitor::FileChangeType type) { > > I am still trying to fully understand the patch but would it make sense to > have the FileMonitor constructor take a WorkQueue in parameter so that we > can pass m_statisticsQueue ? > This way, our handler would get called on the right thread. This seems like a good idea. I'll make that change once I'm back at my desk.
Brent Fulgham
Comment 47 2017-06-27 18:30:15 PDT
Chris Dumez
Comment 48 2017-06-27 18:57:28 PDT
Comment on attachment 313970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313970&action=review More comments coming. > Source/WebCore/platform/FileMonitor.cpp:31 > +Ref<FileMonitor> FileMonitor::create(const String& path, WTF::Function<void(FileChangeType)>&& modificationHandler, Ref<WorkQueue>& handlerQueue) Ref<WorkQueue>&& Also, the call site would look better if the WorkQueue came as the second parameter. > Source/WebCore/platform/FileMonitor.cpp:33 > + return adoptRef(*new FileMonitor(path, WTFMove(modificationHandler), handlerQueue)); WTFMove(handlerQueue) > Source/WebCore/platform/FileMonitor.cpp:39 > + , m_handlerQueue(handlerQueue.copyRef()) WTFMove(handlerQueue) > Source/WebCore/platform/FileMonitor.h:43 > + enum FileChangeType { "enum class" please. > Source/WebCore/platform/FileMonitor.h:48 > + WEBCORE_EXPORT static Ref<FileMonitor> create(const String&, WTF::Function<void(FileChangeType)>&& modificationHandler, Ref<WorkQueue>&); Call site would look better with lambda as last parameter. > Source/WebCore/platform/FileMonitor.h:63 > + DispatchPtr<dispatch_queue_t> m_dispatchQueue; Why is this still here? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:48 > + LOG(ResourceLoadStatistics, "Failed to open file for monitoring: %s", m_path.utf8().data()); RELEASE_LOG_ERROR ? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:52 > + m_dispatchQueue = adoptDispatch(dispatch_queue_create("File monitoring queue", DISPATCH_QUEUE_SERIAL)); Why is this still here? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:53 > + m_platformMonitor = adoptDispatch(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle, monitorMask, m_dispatchQueue.get())); Should probably be using m_handlerQueue.dispatchQueue(), not m_dispatchQueue. > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:55 > + RefPtr<FileMonitor> protectedThis(this); Not needed if comment below is applied. > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:56 > + dispatch_source_set_event_handler(m_platformMonitor.get(), [this, protectedThis, _source = m_platformMonitor.get()] () mutable { protectedThis = makeRef(*this) Can we find a better name than _source ? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:63 > + if (m_modificationHandler) { Why do we even start monitoring if there is no notification handler? startMonitoring() should probably return early if !m_modificationHandler. > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:64 > + m_handlerQueue->dispatch([this, protectedThis = WTFMove(protectedThis)] () mutable { I am assuming the event handler may get called multiple times, therefore, WTFMove() here does not seem safe. You probably want protectedThis.copyRef(). We would probably add testing for multiple notification? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:71 > + m_handlerQueue->dispatch([this, protectedThis = WTFMove(protectedThis)] () mutable { I am assuming the event handler may get called multiple times, therefore, WTFMove() here does not seem safe. You probably want protectedThis.copyRef(). We would probably add testing for multiple notification? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:78 > + dispatch_source_set_cancel_handler(m_platformMonitor.get(), [_source = m_platformMonitor.get()] { Can we find a better name than _source? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:91 > + dispatch_source_cancel(m_platformMonitor.get()); Shouldn't we reset m_platformMonitor to nullptr after this? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:421 > + }, m_statisticsQueue); m_statisticsQueue.copyRef()
Chris Dumez
Comment 49 2017-06-27 19:18:05 PDT
Comment on attachment 313970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313970&action=review > Source/WebCore/platform/FileMonitor.h:41 > +class FileMonitor : public RefCounted<FileMonitor> { Should be ThreadSafeRefCounted given that you capture RefPtrs to it in lambdas that are passed to other threads. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:64 >> + m_handlerQueue->dispatch([this, protectedThis = WTFMove(protectedThis)] () mutable { > > I am assuming the event handler may get called multiple times, therefore, WTFMove() here does not seem safe. You probably want protectedThis.copyRef(). We would probably add testing for multiple notification? I think you misunderstood my proposal. If we call dispatch_source_create() with with handlerQueue, then I believe this eventHandler lambda will be called on the handlerQueue already and there would be no need to dispatch here. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:162 > + RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] () mutable { This is kind of awkward. Could we always use the FileMonitor from the background thread? This way, there would be no need to do this kind of dispatching. If we cannot always use FileMonitor from the background thread, it probably would not be much work to make it thread safe. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:394 > + if (!deleteFile(resourceLogPath)) Didn't you say in a previous comment this would interfere with our monitoring? I don't see us stopping the monitor before deleting the file. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:432 > + m_statisticsStorageMonitor->stopMonitoring(); Why is this needed? Shouldn't nulling out do the trick already?
Brent Fulgham
Comment 50 2017-06-28 11:37:26 PDT
Comment on attachment 313970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313970&action=review Thanks for all that review. New patch coming shortly. >> Source/WebCore/platform/FileMonitor.cpp:31 >> +Ref<FileMonitor> FileMonitor::create(const String& path, WTF::Function<void(FileChangeType)>&& modificationHandler, Ref<WorkQueue>& handlerQueue) > > Ref<WorkQueue>&& > > Also, the call site would look better if the WorkQueue came as the second parameter. Agreed. >> Source/WebCore/platform/FileMonitor.cpp:33 >> + return adoptRef(*new FileMonitor(path, WTFMove(modificationHandler), handlerQueue)); > > WTFMove(handlerQueue) OK. >> Source/WebCore/platform/FileMonitor.cpp:39 >> + , m_handlerQueue(handlerQueue.copyRef()) > > WTFMove(handlerQueue) OK. >> Source/WebCore/platform/FileMonitor.h:41 >> +class FileMonitor : public RefCounted<FileMonitor> { > > Should be ThreadSafeRefCounted given that you capture RefPtrs to it in lambdas that are passed to other threads. Will fix. >> Source/WebCore/platform/FileMonitor.h:43 >> + enum FileChangeType { > > "enum class" please. OK. >> Source/WebCore/platform/FileMonitor.h:48 >> + WEBCORE_EXPORT static Ref<FileMonitor> create(const String&, WTF::Function<void(FileChangeType)>&& modificationHandler, Ref<WorkQueue>&); > > Call site would look better with lambda as last parameter. OK! >> Source/WebCore/platform/FileMonitor.h:63 >> + DispatchPtr<dispatch_queue_t> m_dispatchQueue; > > Why is this still here? Because I totally misunderstood what you suggested! Fixing. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:53 >> + m_platformMonitor = adoptDispatch(dispatch_source_create(DISPATCH_SOURCE_TYPE_VNODE, handle, monitorMask, m_dispatchQueue.get())); > > Should probably be using m_handlerQueue.dispatchQueue(), not m_dispatchQueue. Yes! I totally bungled that. Fixing. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:56 >> + dispatch_source_set_event_handler(m_platformMonitor.get(), [this, protectedThis, _source = m_platformMonitor.get()] () mutable { > > protectedThis = makeRef(*this) > > Can we find a better name than _source ? Clang gets upset by this for reasons I don't understand: dispatch_source_set_event_handler(m_platformMonitor.get(), [this, protectedThis = makeRef(*this), _source = m_platformMonitor.get()] () mutable { "FileMonitorCocoa.mm:56:64: Call to implicitly-deleted copy constructor of 'const (lambda at FileMonitorCocoa.mm:56:64)' >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:63 >> + if (m_modificationHandler) { > > Why do we even start monitoring if there is no notification handler? startMonitoring() should probably return early if !m_modificationHandler. Good idea. >>> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:64 >>> + m_handlerQueue->dispatch([this, protectedThis = WTFMove(protectedThis)] () mutable { >> >> I am assuming the event handler may get called multiple times, therefore, WTFMove() here does not seem safe. You probably want protectedThis.copyRef(). We would probably add testing for multiple notification? > > I think you misunderstood my proposal. If we call dispatch_source_create() with with handlerQueue, then I believe this eventHandler lambda will be called on the handlerQueue already and there would be no need to dispatch here. That's right -- I misunderstood your proposal. Doing so considerably simplifies things, and makes the code much smaller. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:71 >> + m_handlerQueue->dispatch([this, protectedThis = WTFMove(protectedThis)] () mutable { > > I am assuming the event handler may get called multiple times, therefore, WTFMove() here does not seem safe. You probably want protectedThis.copyRef(). We would probably add testing for multiple notification? We actually don't need to move or copy this, since everything is already in the dispatch queue (after making the changes you suggested). >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:78 >> + dispatch_source_set_cancel_handler(m_platformMonitor.get(), [_source = m_platformMonitor.get()] { > > Can we find a better name than _source? Done. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:91 >> + dispatch_source_cancel(m_platformMonitor.get()); > > Shouldn't we reset m_platformMonitor to nullptr after this? Yes. DispatchPtr didn't understand assigning to nullptr, so I've fixed that. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:162 >> + RunLoop::main().dispatch([this, protectedThis = makeRef(*this)] () mutable { > > This is kind of awkward. Could we always use the FileMonitor from the background thread? This way, there would be no need to do this kind of dispatching. > If we cannot always use FileMonitor from the background thread, it probably would not be much work to make it thread safe. I think the FileMonitor only needs to be used on the background queue. I'll revise things to do it this way. I.e., The FileMonitor object might be created on MainThread, or some other queue, but will also perform its function on the work queue provided to it. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:394 >> + if (!deleteFile(resourceLogPath)) > > Didn't you say in a previous comment this would interfere with our monitoring? I don't see us stopping the monitor before deleting the file. Good catch. I should be calling 'stopMonitoringStatisticsStorage()' here. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:421 >> + }, m_statisticsQueue); > > m_statisticsQueue.copyRef() Fixed. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:432 >> + m_statisticsStorageMonitor->stopMonitoring(); > > Why is this needed? Shouldn't nulling out do the trick already? Yup. Fixed.
Brent Fulgham
Comment 51 2017-06-28 13:45:50 PDT
Chris Dumez
Comment 52 2017-06-28 14:16:03 PDT
Comment on attachment 314054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314054&action=review r=me with a few comments. > Source/WTF/wtf/DispatchPtr.h:74 > + std::exchange(m_ptr, nullptr); What's the point of std::exchange since you do not use the return value? Seems like this should be: auto ptr = std::exchange(m_ptr, nullptr); if (ptr) dispatch_release(ptr); > Source/WebCore/platform/FileMonitor.h:29 > +#include <wtf/Noncopyable.h> Is this really needed? > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:57 > + RefPtr<FileMonitor> protectedThis(this); Not needed if comment below is applied. > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:58 > + dispatch_source_set_event_handler(m_platformMonitor.get(), [this, protectedThis, fileMonitor = m_platformMonitor.get()] () mutable { protectedThis = makeRefPtr(this) (I am assuming makeRef(*this) does not work because this expects an ObjC block) > Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:59 > + // If this is getting called after the monitor was cancelled, just drop the notification. Could we add an assertion here to make sure we are not on the main thread? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:193 > + m_statisticsStorageMonitor = nullptr; I do not think we should call the FileMonitor destructor on the main thread. Since it is constructed and used solely on the statisticsQueue, I think we should destroy it on the statisticsQueue as well. E.g. m_statisticsQueue->dispatch([statisticsStorageMonitor = WTFMove(m_statisticsStorageMonitor)] { }); > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:414 > + if (type == FileMonitor::FileChangeType::Modification) Can we please use a switch() (without default: statement) instead of an if? > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:417 > + clearInMemoryData(); Once a statistics file is deleted by someone else, we clear in memory data but do not null out m_statisticsStorageMonitor. Is that OK? If we want to start monitoring again at some point, startMonitoringStatisticsStorage() will be a no-op because m_statisticsStorageMonitor is not null. > Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:105 > + command.append("echo \""); appendLiteral() > Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:107 > + command.append("\" > "); appendLiteral() > Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:152 > + if (type == FileMonitor::FileChangeType::Modification) switch() > Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:190 > + if (type == FileMonitor::FileChangeType::Modification) switch() > Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:246 > + if (type == FileMonitor::FileChangeType::Modification) switch() > Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:255 > + command.append("rm -f "); appendLiteral()
Brent Fulgham
Comment 53 2017-06-28 14:35:56 PDT
Comment on attachment 314054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314054&action=review >> Source/WTF/wtf/DispatchPtr.h:74 >> + std::exchange(m_ptr, nullptr); > > What's the point of std::exchange since you do not use the return value? Seems like this should be: > auto ptr = std::exchange(m_ptr, nullptr); > if (ptr) > dispatch_release(ptr); OK. Will fix. >> Source/WebCore/platform/FileMonitor.h:29 >> +#include <wtf/Noncopyable.h> > > Is this really needed? No. I'll remove it. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:58 >> + dispatch_source_set_event_handler(m_platformMonitor.get(), [this, protectedThis, fileMonitor = m_platformMonitor.get()] () mutable { > > protectedThis = makeRefPtr(this) (I am assuming makeRef(*this) does not work because this expects an ObjC block) yes! That works. thank you. >> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:59 >> + // If this is getting called after the monitor was cancelled, just drop the notification. > > Could we add an assertion here to make sure we are not on the main thread? Will do. WebCore can use "isMainThread()", right? Or should I use "RunLoop::isMain()" like we do in WK2 code. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:193 >> + m_statisticsStorageMonitor = nullptr; > > I do not think we should call the FileMonitor destructor on the main thread. Since it is constructed and used solely on the statisticsQueue, I think we should destroy it on the statisticsQueue as well. E.g. > m_statisticsQueue->dispatch([statisticsStorageMonitor = WTFMove(m_statisticsStorageMonitor)] { }); Will do. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:414 >> + if (type == FileMonitor::FileChangeType::Modification) > > Can we please use a switch() (without default: statement) instead of an if? Sure! >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:417 >> + clearInMemoryData(); > > Once a statistics file is deleted by someone else, we clear in memory data but do not null out m_statisticsStorageMonitor. Is that OK? > If we want to start monitoring again at some point, startMonitoringStatisticsStorage() will be a no-op because m_statisticsStorageMonitor is not null. Good point. This should be nulled out in that case. We will start monitoring again when we decide to store our data on disk again, at which point we sync with whatever happens to be there and start monitoring after that. In fact, it doesn't even make sense to keep the observer running, because the file handle we are observing no longer points to anything, and will never fire again so it's just wasteful to keep it around. >> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:105 >> + command.append("echo \""); > > appendLiteral() Will do. >> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:107 >> + command.append("\" > "); > > appendLiteral() Ditto. >> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:152 >> + if (type == FileMonitor::FileChangeType::Modification) > > switch() OK. >> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:190 >> + if (type == FileMonitor::FileChangeType::Modification) > > switch() OK. >> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:246 >> + if (type == FileMonitor::FileChangeType::Modification) > > switch() OK. >> Tools/TestWebKitAPI/Tests/WebCore/FileMonitor.cpp:255 >> + command.append("rm -f "); > > appendLiteral() OK.
Chris Dumez
Comment 54 2017-06-28 14:38:49 PDT
Comment on attachment 314054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314054&action=review >>> Source/WebCore/platform/cocoa/FileMonitorCocoa.mm:59 >>> + // If this is getting called after the monitor was cancelled, just drop the notification. >> >> Could we add an assertion here to make sure we are not on the main thread? > > Will do. WebCore can use "isMainThread()", right? Or should I use "RunLoop::isMain()" like we do in WK2 code. Actually, let's do the check on client side and use RunLoop::isMain() since the client is in WK2.
Brent Fulgham
Comment 55 2017-06-28 16:21:13 PDT
Csaba Osztrogonác
Comment 56 2017-06-29 07:07:00 PDT
(In reply to Brent Fulgham from comment #55) > Committed r218901: <http://trac.webkit.org/changeset/218901> It broke the Apple Mac cmake build: Undefined symbols for architecture x86_64: "WebCore::FileMonitor::stopMonitoring()", referenced from: WebCore::FileMonitor::~FileMonitor() in FileMonitor.cpp.o ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation)
Brent Fulgham
Comment 57 2017-06-29 09:44:35 PDT
Note: I disabled the test on iOS, since that platform does not allow the ‘system’ API call to be used to drive a second process to poke at the test files. Committed r218906: <http://trac.webkit.org/changeset/218906>
Brent Fulgham
Comment 58 2017-06-29 09:48:49 PDT
Build fix for Apple CMake build committed in r218932. Committed r218932: <http://trac.webkit.org/changeset/218932>
Note You need to log in before you can comment on or make changes to this bug.