<rdar://problem/16660724> When we quit safari, we sometimes kill the UIprocess as any uncommitted local storage transactions are committed. This patch ensure that upon quitting we do all the necessary cleanup first before exiting.
Created attachment 233329 [details] patch
Created attachment 233330 [details] patch
Comment on attachment 233330 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=233330&action=review > Source/WebKit2/UIProcess/WebContext.cpp:723 > +void WebContext::closeConnectionsInResponseToQuit() I think calling this applicationWillTerminate is better. > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:611 > +void StorageManager::closeConnectionsInResponseToQuit() > +{ > + Vector<std::pair<RefPtr<IPC::Connection>, uint64_t>> connectionAndStorageMapIDPairsToRemove; > + HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>> storageAreasByConnection = m_storageAreasByConnection; > + for (HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>>::const_iterator it = storageAreasByConnection.begin(), end = storageAreasByConnection.end(); it != end; ++it) { > + it->value->removeListener(it->key.first.get(), it->key.second); > + connectionAndStorageMapIDPairsToRemove.append(it->key); > + } > + > + for (size_t i = 0; i < connectionAndStorageMapIDPairsToRemove.size(); ++i) > + m_storageAreasByConnection.remove(connectionAndStorageMapIDPairsToRemove[i]); > + > + m_closeMutex.unlock(); > +} How is this ensuring that everything is written to disk? > Source/WebKit2/UIProcess/Storage/StorageManager.h:115 > + std::mutex m_closeMutex; A mutex is the wrong synchronization primitive here. You should use a BinarySemaphore instead. For example, StorageManager::applicationWillTerminate() could do something like: void StorageManager::applicationWillTerminate() { BinarySemaphore semaphore; m_queue->dispatch([this, &semaphore]) { // Do cleanup work. semaphore.signal(); }); semaphore.wait();
(In reply to comment #3) > (From update of attachment 233330 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233330&action=review > > > Source/WebKit2/UIProcess/WebContext.cpp:723 > > +void WebContext::closeConnectionsInResponseToQuit() > > I think calling this applicationWillTerminate is better. > > > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:611 > > +void StorageManager::closeConnectionsInResponseToQuit() > > +{ > > + Vector<std::pair<RefPtr<IPC::Connection>, uint64_t>> connectionAndStorageMapIDPairsToRemove; > > + HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>> storageAreasByConnection = m_storageAreasByConnection; > > + for (HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>>::const_iterator it = storageAreasByConnection.begin(), end = storageAreasByConnection.end(); it != end; ++it) { > > + it->value->removeListener(it->key.first.get(), it->key.second); > > + connectionAndStorageMapIDPairsToRemove.append(it->key); > > + } > > + > > + for (size_t i = 0; i < connectionAndStorageMapIDPairsToRemove.size(); ++i) > > + m_storageAreasByConnection.remove(connectionAndStorageMapIDPairsToRemove[i]); > > + > > + m_closeMutex.unlock(); > > +} > > How is this ensuring that everything is written to disk? > > > Source/WebKit2/UIProcess/Storage/StorageManager.h:115 > > + std::mutex m_closeMutex; > > A mutex is the wrong synchronization primitive here. You should use a BinarySemaphore instead. For example, StorageManager::applicationWillTerminate() could do something like: > > void StorageManager::applicationWillTerminate() > { > BinarySemaphore semaphore; > m_queue->dispatch([this, &semaphore]) { > // Do cleanup work. > semaphore.signal(); > }); > > semaphore.wait(); Would it not make sense, in addition to the semaphore, to use a mutex with the background thread to make sure that closeConnectionsInResponseToQuit and invalidateConnectionInternal don't modify the m_storageAreasByConnection map at the same time?
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 233330 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=233330&action=review > > > > > Source/WebKit2/UIProcess/WebContext.cpp:723 > > > +void WebContext::closeConnectionsInResponseToQuit() > > > > I think calling this applicationWillTerminate is better. > > > > > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:611 > > > +void StorageManager::closeConnectionsInResponseToQuit() > > > +{ > > > + Vector<std::pair<RefPtr<IPC::Connection>, uint64_t>> connectionAndStorageMapIDPairsToRemove; > > > + HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>> storageAreasByConnection = m_storageAreasByConnection; > > > + for (HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>>::const_iterator it = storageAreasByConnection.begin(), end = storageAreasByConnection.end(); it != end; ++it) { > > > + it->value->removeListener(it->key.first.get(), it->key.second); > > > + connectionAndStorageMapIDPairsToRemove.append(it->key); > > > + } > > > + > > > + for (size_t i = 0; i < connectionAndStorageMapIDPairsToRemove.size(); ++i) > > > + m_storageAreasByConnection.remove(connectionAndStorageMapIDPairsToRemove[i]); > > > + > > > + m_closeMutex.unlock(); > > > +} > > > > How is this ensuring that everything is written to disk? > > > > > Source/WebKit2/UIProcess/Storage/StorageManager.h:115 > > > + std::mutex m_closeMutex; > > > > A mutex is the wrong synchronization primitive here. You should use a BinarySemaphore instead. For example, StorageManager::applicationWillTerminate() could do something like: > > > > void StorageManager::applicationWillTerminate() > > { > > BinarySemaphore semaphore; > > m_queue->dispatch([this, &semaphore]) { > > // Do cleanup work. > > semaphore.signal(); > > }); > > > > semaphore.wait(); > > Would it not make sense, in addition to the semaphore, to use a mutex with the background thread to make sure that closeConnectionsInResponseToQuit and invalidateConnectionInternal don't modify the m_storageAreasByConnection map at the same time? I retract my statement. It's the same work queue, so that can't happen, duh.,
Created attachment 233353 [details] patch
Created attachment 233354 [details] patch
Comment on attachment 233354 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=233354&action=review > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:597 > + HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>> storageAreasByConnection = m_storageAreasByConnection; No need to copy the HashMap here. > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:598 > + for (HashMap<std::pair<RefPtr<IPC::Connection>, uint64_t>, RefPtr<StorageArea>>::const_iterator it = storageAreasByConnection.begin(), end = storageAreasByConnection.end(); it != end; ++it) { This should use a modern for loop. > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:604 > + for (size_t i = 0; i < connectionAndStorageMapIDPairsToRemove.size(); ++i) > + m_storageAreasByConnection.remove(connectionAndStorageMapIDPairsToRemove[i]); This should use a modern for loop, and I think a comment stating that removing the storage areas causes the changes to be flushed.
committed: http://trac.webkit.org/changeset/170156