Bug 174435

Created attachment 315290 [details] Patch

Created attachment 315304 [details] Patch

Created attachment 315305 [details] Patch

Created attachment 315327 [details] Patch

Created attachment 315330 [details] Patch

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

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

Created attachment 315333 [details] Patch

Created attachment 315349 [details] Patch

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).

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.

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

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

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.

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.

Comment on attachment 315349 [details] Patch Clearing flags on attachment: 315349 Committed r219453: <http://trac.webkit.org/changeset/219453>

All reviewed patches have been landed. Closing bug.

Reverted r219453 for reason: Seems to cause some crashes on the bots Committed r219467: <http://trac.webkit.org/changeset/219467>

Created attachment 315377 [details] Patch

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.

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.

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

Comment on attachment 315377 [details] Patch r=me

Comment on attachment 315377 [details] Patch Clearing flags on attachment: 315377 Committed r219475: <http://trac.webkit.org/changeset/219475>

All reviewed patches have been landed. Closing bug.