Bug 174435 – Moved filesystem code out of WebResourceLoadStatisticsStore class

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Moved filesystem code out of WebResourceLoadStatisticsStore class to decrease complexity.

[Chris Dumez]

Created attachment 315290 [details]
Patch

[Chris Dumez]

Created attachment 315304 [details]
Patch

[Chris Dumez]

Created attachment 315305 [details]
Patch

[Chris Dumez]

Created attachment 315327 [details]
Patch

[Chris Dumez]

Created attachment 315330 [details]
Patch

[Build Bot]

Comment on attachment 315330 [details]
Patch

Attachment 315330 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4111327

New failing tests:
storage/websql/execute-sql-rowsAffected.html
imported/w3c/web-platform-tests/IndexedDB/large-nested-cloning.html

[Build Bot]

Created attachment 315332 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Chris Dumez]

Created attachment 315333 [details]
Patch

[Chris Dumez]

Created attachment 315349 [details]
Patch

[Brent Fulgham]

Comment on attachment 315349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315349&action=review

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:209
> +

In the original implementation I made a call to sync the memory store with the on-disk store here. I still think this is necessary, even though we monitor file change/file delete events because we stop monitoring file events after delete (since the we can't make a file handle to a non-existent file).

A second process could write a new copy of the file while we are running, which means we could lose data. A potentially cleaner approach might be to monitor the directory holding the statistics file, which would tell us when a new files was created.

I took the easier approach of checking for data before writing, and syncing before conducting the write.

I think this change loses the ability to react to new data appearing on disk while we are not monitoring the disk.

I guess we could fix this in a separate patch (adding a new handler for directory changes).

[Chris Dumez]

Comment on attachment 315349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315349&action=review

>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:209
>> +
> 
> In the original implementation I made a call to sync the memory store with the on-disk store here. I still think this is necessary, even though we monitor file change/file delete events because we stop monitoring file events after delete (since the we can't make a file handle to a non-existent file).
> 
> A second process could write a new copy of the file while we are running, which means we could lose data. A potentially cleaner approach might be to monitor the directory holding the statistics file, which would tell us when a new files was created.
> 
> I took the easier approach of checking for data before writing, and syncing before conducting the write.
> 
> I think this change loses the ability to react to new data appearing on disk while we are not monitoring the disk.
> 
> I guess we could fix this in a separate patch (adding a new handler for directory changes).

I wanted to discuss the few minor behavior changes with you today. Yes, there was a call to sync the memory cache before writing (using syncWithExistingStatisticsStorageIfNeeded) and I dropped it. The reason I dropped it it that it seemed to be a no-op. If we're in this function, then it means we have in memory data to write. syncWithExistingStatisticsStorageIfNeeded() was calling refreshFromDisk(), which would call populateFromDecoder(). populateFromDecoder() did an early return if we had any data. Even if populateFromDecoder() did not do an early return, then we'd still be in trouble I believe because this "sync" before writing would override our new data.

I understand now why the existing code was in place, however, unless I am missing something, I don't think it did what it was supposed to do.

[Build Bot]

Comment on attachment 315349 [details]
Patch

Attachment 315349 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4113849

New failing tests:
storage/indexeddb/modern/new-database-after-user-delete.html

[Build Bot]

Created attachment 315351 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Brent Fulgham]

Comment on attachment 315349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315349&action=review

>>> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:209
>>> +
>> 
>> In the original implementation I made a call to sync the memory store with the on-disk store here. I still think this is necessary, even though we monitor file change/file delete events because we stop monitoring file events after delete (since the we can't make a file handle to a non-existent file).
>> 
>> A second process could write a new copy of the file while we are running, which means we could lose data. A potentially cleaner approach might be to monitor the directory holding the statistics file, which would tell us when a new files was created.
>> 
>> I took the easier approach of checking for data before writing, and syncing before conducting the write.
>> 
>> I think this change loses the ability to react to new data appearing on disk while we are not monitoring the disk.
>> 
>> I guess we could fix this in a separate patch (adding a new handler for directory changes).
> 
> I wanted to discuss the few minor behavior changes with you today. Yes, there was a call to sync the memory cache before writing (using syncWithExistingStatisticsStorageIfNeeded) and I dropped it. The reason I dropped it it that it seemed to be a no-op. If we're in this function, then it means we have in memory data to write. syncWithExistingStatisticsStorageIfNeeded() was calling refreshFromDisk(), which would call populateFromDecoder(). populateFromDecoder() did an early return if we had any data. Even if populateFromDecoder() did not do an early return, then we'd still be in trouble I believe because this "sync" before writing would override our new data.
> 
> I understand now why the existing code was in place, however, unless I am missing something, I don't think it did what it was supposed to do.

Sounds good. I propose creating a separate issue to do add a directory monitor when we detect a file delete, which will go away when we see the file come back (or we write a file). Then we can handle the sync of existing data when new data is written, not at the moment we are about to write.

[Chris Dumez]

mac-debug crash seems unrelated:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000011032ddd7 WTFCrash + 39 (Assertions.cpp:293)
1   com.apple.WebCore             	0x00000001186a6176 WebCore::IDBServer::IDBServer::closeAndTakeUniqueIDBDatabase(WebCore::IDBServer::UniqueIDBDatabase&) + 278 (IDBServer.cpp:180)
2   com.apple.WebCore             	0x000000011a2ecd82 WebCore::IDBServer::UniqueIDBDatabase::immediateCloseForUserDelete() + 1874 (UniqueIDBDatabase.cpp:1869)
3   com.apple.WebCore             	0x00000001186a7c39 WebCore::IDBServer::IDBServer::closeAndDeleteDatabasesModifiedSince(std::__1::chrono::time_point<std::__1::chrono::system_clock, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000000l> > >, WTF::Function<void ()>&&) + 1001 (IDBServer.cpp:556)
4   com.apple.WebKitLegacy        	0x000000012265d452 WebDatabaseProvider::deleteAllDatabases() + 498 (WebDatabaseProvider.cpp:62)
5   com.apple.WebKitLegacy        	0x000000012265b6cd -[WebDatabaseManager deleteAllIndexedDatabases] + 29 (WebDatabaseManager.mm:160)
6   DumpRenderTree                	0x000000010e910053 TestRunner::clearAllDatabases() + 83 (TestRunnerMac.mm:207)
7   DumpRenderTree                	0x000000010e8fa7d6 clearAllDatabasesCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**) + 54 (TestRunner.cpp:476)
8   com.apple.JavaScriptCore      	0x000000010fce4629 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*) + 553 (APICallbackFunction.h:63)
9   com.apple.JavaScriptCore      	0x000000010fe9aaa3 JSC::LLInt::handleHostCall(JSC::ExecState*, JSC::Instruction*, JSC::JSValue, JSC::CodeSpecializationKind) + 387 (LLIntSlowPaths.cpp:1244)
10  com.apple.JavaScriptCore      	0x000000010fe9bce0 JSC::LLInt::setUpCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind, JSC::JSValue, JSC::LLIntCallLinkInfo*) + 256 (LLIntSlowPaths.cpp:1293)
11  com.apple.JavaScriptCore      	0x000000010fe9b8b6 JSC::LLInt::genericCall(JSC::ExecState*, JSC::Instruction*, JSC::CodeSpecializationKind) + 230 (LLIntSlowPaths.cpp:1360)
12  com.apple.JavaScriptCore      	0x000000010fe97c99 llint_slow_path_call + 169 (LLIntSlowPaths.cpp:1367)
13  com.apple.JavaScriptCore      	0x000000010fea6c41 llint_entry + 30571

IndexedDB is crashy these days.

[Chris Dumez]

Comment on attachment 315349 [details]
Patch

Clearing flags on attachment: 315349

Committed r219453: <http://trac.webkit.org/changeset/219453>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reverted r219453 for reason:

Seems to cause some crashes on the bots

Committed r219467: <http://trac.webkit.org/changeset/219467>

[Chris Dumez]

Created attachment 315377 [details]
Patch

[Chris Dumez]

Comment on attachment 315349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315349&action=review

> Source/WebKit2/UIProcess/Storage/ResourceLoadStatisticsPersistentStorage.cpp:90
> +    m_memoryStore.statisticsQueue().dispatch([this] {

This was racy because the ResourceLoadStatisticsPersistentStorage object is constructed on the main thread in the WebResourceLoadStatisticsStore initialization list.
It was possible for the lambda to get executed before all the WebResourceLoadStatisticsStore members were properly initialized.

[Chris Dumez]

Comment on attachment 315377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315377&action=review

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:93
> +        m_persistentStorage.initialize();

So I now do the work here, where it used to be done, after all data members have been initialized.

[Chris Dumez]

For reference, the crash looked like:
Thread 5 Crashed:: Dispatch queue: WebResourceLoadStatisticsStore Process Data Queue
0   com.apple.JavaScriptCore      	0x000000010fb686a4 WTFCrash + 36 (Assertions.cpp:293)
1   com.apple.WebKit              	0x0000000114c132ec WTF::Deque<WTF::WallTime, 0ul>::checkValidity() const + 316 (Deque.h:253)
2   com.apple.WebKit              	0x0000000114c0c8f9 WTF::Deque<WTF::WallTime, 0ul>::clear() + 25 (Deque.h:381)
3   com.apple.WebKit              	0x0000000114c0c2f1 WebKit::WebResourceLoadStatisticsStore::clearInMemory() + 113 (WebResourceLoadStatisticsStore.cpp:539)
4   com.apple.WebKit              	0x0000000114c0bef5 WebKit::WebResourceLoadStatisticsStore::resetDataFromDecoder(WebCore::KeyedDecoder&) + 101 (WebResourceLoadStatisticsStore.cpp:487)
5   com.apple.WebKit              	0x0000000114336f0b WebKit::ResourceLoadStatisticsPersistentStorage::populateMemoryStoreFromDisk() + 411 (ResourceLoadStatisticsPersistentStorage.cpp:197)
6   com.apple.WebKit              	0x000000011433815c WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage(WebKit::WebResourceLoadStatisticsStore&, WTF::String const&)::$_0::operator()() const + 28 (ResourceLoadStatisticsPersistentStorage.cpp:92)
7   com.apple.WebKit              	0x0000000114338119 WTF::Function<void ()>::CallableWrapper<WebKit::ResourceLoadStatisticsPersistentStorage::ResourceLoadStatisticsPersistentStorage(WebKit::WebResourceLoadStatisticsStore&, WTF::String const&)::$_0>::call() + 25 (Function.h:102)
8   com.apple.JavaScriptCore      	0x000000010fb98e1e WTF::Function<void ()>::operator()() const + 94 (Function.h:56)
9   com.apple.JavaScriptCore      	0x000000010fbede59 WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0::operator()() const + 25 (WorkQueueCocoa.cpp:37)
10  com.apple.JavaScriptCore      	0x000000010fbede30 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::operator()(void*) const + 32 (BlockPtr.h:87)
11  com.apple.JavaScriptCore      	0x000000010fbede08 WTF::BlockPtr<void ()> WTF::BlockPtr<void ()>::fromCallable<WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0>(WTF::WorkQueue::dispatch(WTF::Function<void ()>&&)::$_0)::'lambda'(void*)::__invoke(void*) + 24 (BlockPtr.h:86)
12  libdispatch.dylib             	0x00007fff9422c524 _dispatch_call_block_and_release + 12
13  libdispatch.dylib             	0x00007fff942238fc _dispatch_client_callout + 8
14  libdispatch.dylib             	0x00007fff942399a0 _dispatch_queue_serial_drain + 896
15  libdispatch.dylib             	0x00007fff9422c306 _dispatch_queue_invoke + 1046
16  libdispatch.dylib             	0x00007fff942256b5 _dispatch_root_queue_drain + 476
17  libdispatch.dylib             	0x00007fff9422548c _dispatch_worker_thread3 + 99
18  libsystem_pthread.dylib       	0x00007fff944725a2 _pthread_wqthread + 1299
19  libsystem_pthread.dylib       	0x00007fff9447207d start_wqthread + 13

[Brent Fulgham]

Comment on attachment 315377 [details]
Patch

r=me

[Chris Dumez]

Comment on attachment 315377 [details]
Patch

Clearing flags on attachment: 315377

Committed r219475: <http://trac.webkit.org/changeset/219475>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.