WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(18.83 KB, patch)
2017-06-24 16:09 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch v3 - use dispatch routines
(20.84 KB, patch)
2017-06-25 10:29 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch v4: Correct iOS build.
(20.71 KB, patch)
2017-06-25 13:35 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch v5: Correct file deletion behavior.
(20.66 KB, patch)
2017-06-26 09:50 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(46.26 KB, patch)
2017-06-27 10:43 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(46.27 KB, patch)
2017-06-27 10:46 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(47.20 KB, patch)
2017-06-27 14:05 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Complete Patch (with initial test case)
(47.16 KB, patch)
2017-06-27 14:20 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch (Rebaselined).
(47.36 KB, patch)
2017-06-27 14:32 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
Patch
(49.00 KB, patch)
2017-06-27 17:53 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch (Fix GTK/WPE Builds)
(49.53 KB, patch)
2017-06-27 17:58 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(50.26 KB, patch)
2017-06-27 18:30 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(58.23 KB, patch)
2017-06-28 13:45 PDT
,
Brent Fulgham
cdumez
: review+
cdumez
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(15)
View All
Add attachment
proposed patch, testcase, etc.
Brent Fulgham
Comment 1
2017-06-23 17:21:46 PDT
<
rdar://problem/32937842
>
Brent Fulgham
Comment 2
2017-06-23 18:02:13 PDT
Created
attachment 313763
[details]
Patch
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
Created
attachment 313791
[details]
Patch
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
Created
attachment 313926
[details]
Patch
Brent Fulgham
Comment 26
2017-06-27 10:46:26 PDT
Created
attachment 313927
[details]
Patch
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
Created
attachment 313938
[details]
Patch
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
Created
attachment 313967
[details]
Patch
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
Created
attachment 313970
[details]
Patch
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
Created
attachment 314054
[details]
Patch
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
Committed
r218901
: <
http://trac.webkit.org/changeset/218901
>
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.
Top of Page
Format For Printing
XML
Clone This Bug