If the ResourceLoadStatistics data file is deleted on disk, we should monitor the data directory to see if a new ones is created by a separate process. If so, we should sync with that data and begin watching the new file for changes. This replaces the buggy logic I originally wrote that would attempt to sync when we wanted to write to disk.
<rdar://problem/33322189>
Created attachment 315478 [details] Patch
Comment on attachment 315478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315478&action=review > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:137 > + monitorDirectoryForNewStatistics(); This is insufficient. You have the same issue when populateMemoryStoreFromDisk() is called and the file does not exist yet. > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:145 > + String storagePath = this->storageDirectoryPath(); Don't need the the this-> > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:150 > + if (m_fileMonitor || type == FileMonitor::FileChangeType::Removal) { m_fileMonitor being non-null should not be possible? What does removal mean here. Is this called when the directory gets removed. What if a file was removed in the folder? > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:163 > + refreshMemoryStoreFromDisk(); Do we want refreshMemoryStoreFromDisk() or populateMemoryStoreFromDisk() here? > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:68 > + std::unique_ptr<WebCore::FileMonitor> m_directoryMonitor; Do we really need a second FileMonitor member, it seems to be m_fileMonitor and m_directoryMonitor are mutually exclusive and we could thus reuse the same member.
Comment on attachment 315478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315478&action=review > Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:148 > + m_directoryMonitor = std::make_unique<FileMonitor>(storagePath, m_memoryStore.statisticsQueue(), [this] (FileMonitor::FileChangeType type) { What if the folder does not exist yet. You only call makeAllDirectories() when actually writing data to disk at the moment. I think we should probably call makeAllDirectories() here before we start monitoring as well. >> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:150 >> + if (m_fileMonitor || type == FileMonitor::FileChangeType::Removal) { > > m_fileMonitor being non-null should not be possible? > > What does removal mean here. Is this called when the directory gets removed. What if a file was removed in the folder? I looked it up and it looks like we get a DISPATCH_VNODE_WRITE for file additions and removals from the folder. DISPATCH_VNODE_DELETE must mean that the folder has been removed then, in which case I case it is ok to stop monitoring.
Comment on attachment 315478 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315478&action=review >> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:137 >> + monitorDirectoryForNewStatistics(); > > This is insufficient. You have the same issue when populateMemoryStoreFromDisk() is called and the file does not exist yet. Oh, good point! I'll add the call there, too. >> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:145 >> + String storagePath = this->storageDirectoryPath(); > > Don't need the the this-> Right -- I'll remove. >> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:148 >> + m_directoryMonitor = std::make_unique<FileMonitor>(storagePath, m_memoryStore.statisticsQueue(), [this] (FileMonitor::FileChangeType type) { > > What if the folder does not exist yet. You only call makeAllDirectories() when actually writing data to disk at the moment. I think we should probably call makeAllDirectories() here before we start monitoring as well. Good idea. I'll fix that. >>> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:150 >>> + if (m_fileMonitor || type == FileMonitor::FileChangeType::Removal) { >> >> m_fileMonitor being non-null should not be possible? >> >> What does removal mean here. Is this called when the directory gets removed. What if a file was removed in the folder? > > I looked it up and it looks like we get a DISPATCH_VNODE_WRITE for file additions and removals from the folder. DISPATCH_VNODE_DELETE must mean that the folder has been removed then, in which case I case it is ok to stop monitoring. Yes -- that's right. I was worried about a case where we are monitoring for an external process, and our own process decides to write a file. Since VNODE events are not filtered by process, we might see our own file create and believe we should take some action. So, if the monitor sees that we are already monitoring a file, it should just exit. >> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:163 >> + refreshMemoryStoreFromDisk(); > > Do we want refreshMemoryStoreFromDisk() or populateMemoryStoreFromDisk() here? I think we want to refresh, since the user might have done some browsing since the file was deleted, but before the current process writes its own content. So we don't want to lose observations made in the running process just because an external process wrote some data. >> Source/WebKit/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.h:68 >> + std::unique_ptr<WebCore::FileMonitor> m_directoryMonitor; > > Do we really need a second FileMonitor member, it seems to be m_fileMonitor and m_directoryMonitor are mutually exclusive and we could thus reuse the same member. Yes, I was thinking that might be the case as well. I'll try that approach.
Created attachment 315495 [details] Patch
Comment on attachment 315495 [details] Patch r=me if the bots are happy.
Committed r219530: <http://trac.webkit.org/changeset/219530>
*** Bug 174466 has been marked as a duplicate of this bug. ***