Stop StorageManager when network process is ready to suspend

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

Suspend StorageManager threads when network process is in preparation to suspend and resume the threads when network process wakes up.

[Sihui Liu]

<rdar://problem/49683172>

[Sihui Liu]

Created attachment 370532 [details]
Patch

[youenn fablet]

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?

[Geoffrey Garen]

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.

[Sihui Liu]

Created attachment 370583 [details]
Patch

[Sihui Liu]

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

[Sihui Liu]

Created attachment 370587 [details]
Patch

[Geoffrey Garen]

Comment on attachment 370587 [details]
Patch

r=me

[Sihui Liu]

Created attachment 370867 [details]
Patch

[Sihui Liu]

Test added.

[youenn fablet]

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

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 370887 [details]
Patch

[Sihui Liu]

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

[youenn fablet]

Comment on attachment 370887 [details]
Patch

looks much better indeed!

[EWS Watchlist]

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

[EWS Watchlist]

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

[Sihui Liu]

Created attachment 370976 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370976 [details]
Patch for landing

Clearing flags on attachment: 370976

Committed r245904: <https://trac.webkit.org/changeset/245904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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.