| Summary: | Teach ResourceLoadStatistics to recognize changes in the file system | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||||||||||||||||||||
| Component: | WebKit Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | achristensen, beidson, benjamin, bfulgham, buildbot, cdumez, cgarcia, cmarcelo, dbates, ggaren, japhet, koivisto, ossy, rniwa, sam, webkit-bug-importer, wilander | ||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 173845 | ||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||
|
Description
Brent Fulgham
2017-06-23 17:21:22 PDT
Created attachment 313763 [details]
Patch
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. Created attachment 313791 [details]
Patch
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(). 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. 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? 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. Created attachment 313809 [details]
Patch v3 - use dispatch routines
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.
Drat. I left the stupid CoreServices include in the file! Created attachment 313813 [details]
Patch v4: Correct iOS build.
Created attachment 313847 [details]
Patch v5: Correct file deletion behavior.
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.
Note: Test infrastructure to support automated testing of these changes are being done under Bug 173845. 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? 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? 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. (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. :\ (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. 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. 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. (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”. (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). Created attachment 313926 [details]
Patch
Created attachment 313927 [details]
Patch
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? 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 }. 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. Created attachment 313938 [details]
Patch
Created attachment 313942 [details]
Complete Patch (with initial test case)
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.
Created attachment 313945 [details]
Patch (Rebaselined).
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. 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! 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? 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. 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? 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. 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 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
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 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
Created attachment 313967 [details]
Patch
Created attachment 313968 [details]
Patch (Fix GTK/WPE Builds)
(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. Created attachment 313970 [details]
Patch
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() 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? 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. Created attachment 314054 [details]
Patch
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() 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. 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. Committed r218901: <http://trac.webkit.org/changeset/218901> (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) 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> Build fix for Apple CMake build committed in r218932. Committed r218932: <http://trac.webkit.org/changeset/218932> |