Suspend StorageManager threads when network process is in preparation to suspend and resume the threads when network process wakes up.
<rdar://problem/49683172>
Created attachment 370532 [details] Patch
Comment on attachment 370532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370532&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2035 > + for (auto& IDSession : m_networkSessions) { s/IDSession/session > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2036 > + if (!IDSession.key.isEphemeral()) We could do that isEphemeral check inside the call to waitUntilSuspend like done for IDB. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:948 > + BinarySemaphore semaphore; Other cleanup methods are taking a lambda so that we complete it after going to the background thread, cleaning things and hopping back to main thread to call the completion handler. With this approach, we will clean sequentially each storage manager. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:949 > + m_queue->dispatch([this, &semaphore] { There is the case where the network session gets deleted before resuming happens, hence StorageManager::resume() is never called. This is probably fine. There is the case where semaphore.signal() is called, the background thread gets paused, main thread continues and network session gets deleted hence m_resumeSemaphore as well. Is there a possibility of a UAF for m_resumeSemaphore?
Comment on attachment 370532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370532&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2036 >> + if (!IDSession.key.isEphemeral()) > > We could do that isEphemeral check inside the call to waitUntilSuspend like done for IDB. Is this isEphemeral check required? All things considered, it's best if private sessions and non-private sessions behave the exact same way.
Created attachment 370583 [details] Patch
(In reply to Geoffrey Garen from comment #4) > Comment on attachment 370532 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370532&action=review > > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2036 > >> + if (!IDSession.key.isEphemeral()) > > > > We could do that isEphemeral check inside the call to waitUntilSuspend like done for IDB. > > Is this isEphemeral check required? All things considered, it's best if > private sessions and non-private sessions behave the exact same way. Only non-ephemeral session's local storage will hold lock to database files. Also we should have only one effective persistent session in network process currently: stopping one storage manager should cost less time than stopping multiple.
Created attachment 370587 [details] Patch
Comment on attachment 370587 [details] Patch r=me
Created attachment 370867 [details] Patch
Test added.
Comment on attachment 370867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370867&action=review > Source/WebKit/NetworkProcess/NetworkSession.cpp:93 > + m_storageManager->waitUntilResume(); I wonder whether you could not make work your previous binary semaphore variant with something like: 1. ref the storage manager as done in this patch when dispatching in suspend. 2. signal the suspend semaphore in NetworkSession destructor so that the dispatched task will complete and release the StorageManager ref. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.h:36 > +#include <wtf/threads/BinarySemaphore.h> Lock.h > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:129 > + [processPool.get() _sendNetworkProcessDidResume]; Can we beef up the test a little bit, something like: 1. network process is not suspended 2. execute JS in page1 to store value 3. execute JS in page2 to retrieve value and verify it is the right one. 4. suspend network process 5. execute JS in page 1 to store a new value 6. execute JS in page 2 to retrieve value and verify it is not page1 value 7. resume network process 8. execute JS in page 2 to retrieve value and verify it is the right one
Comment on attachment 370867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370867&action=review >> Source/WebKit/NetworkProcess/NetworkSession.cpp:93 >> + m_storageManager->waitUntilResume(); > > I wonder whether you could not make work your previous binary semaphore variant with something like: > 1. ref the storage manager as done in this patch when dispatching in suspend. > 2. signal the suspend semaphore in NetworkSession destructor so that the dispatched task will complete and release the StorageManager ref. "Signal the suspend semaphore": we should signal the semaphore only when the thread is being suspended and make sure the thread won't get suspended after session is destroyed. This means we need to keep the state of work queue, which requires lock as it's accessed by different threads? >> Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:129 >> + [processPool.get() _sendNetworkProcessDidResume]; > > Can we beef up the test a little bit, something like: > 1. network process is not suspended > 2. execute JS in page1 to store value > 3. execute JS in page2 to retrieve value and verify it is the right one. > 4. suspend network process > 5. execute JS in page 1 to store a new value > 6. execute JS in page 2 to retrieve value and verify it is not page1 value > 7. resume network process > 8. execute JS in page 2 to retrieve value and verify it is the right one Sure, will do.
Created attachment 370887 [details] Patch
(In reply to Sihui Liu from comment #12) > Comment on attachment 370867 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370867&action=review > > >> Source/WebKit/NetworkProcess/NetworkSession.cpp:93 > >> + m_storageManager->waitUntilResume(); > > > > I wonder whether you could not make work your previous binary semaphore variant with something like: > > 1. ref the storage manager as done in this patch when dispatching in suspend. > > 2. signal the suspend semaphore in NetworkSession destructor so that the dispatched task will complete and release the StorageManager ref. > > "Signal the suspend semaphore": we should signal the semaphore only when the > thread is being suspended and make sure the thread won't get suspended after > session is destroyed. > This means we need to keep the state of work queue, which requires lock as > it's accessed by different threads? > In the newly uploaded patch I use enum instead of two booleans and conditions, making this look simpler...
Comment on attachment 370887 [details] Patch looks much better indeed!
Comment on attachment 370887 [details] Patch Attachment 370887 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12321979 New failing tests: fast/block/float/float-with-anonymous-previous-sibling.html
Created attachment 370894 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 370976 [details] Patch for landing
Comment on attachment 370976 [details] Patch for landing Clearing flags on attachment: 370976 Committed r245904: <https://trac.webkit.org/changeset/245904>
All reviewed patches have been landed. Closing bug.
Comment on attachment 370976 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370976&action=review > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:950 > + return; Fails to call completion handler. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:954 > + return; Fails to call completion handler. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:961 > + completionHandler(); Calls completion handler on the wrong thread.