Bug 134042

Summary: Don't kill the UIProcess until all local storage transactions have been committed
Product: WebKit Reporter: Roger Fong <roger_fong>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, beidson, jonlee, roger_fong
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
andersca: review-
patch
none
patch andersca: review+

Description Roger Fong 2014-06-18 14:48:55 PDT
<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.
Comment 1 Roger Fong 2014-06-18 15:01:49 PDT
Created attachment 233329 [details]
patch
Comment 2 Roger Fong 2014-06-18 15:06:59 PDT
Created attachment 233330 [details]
patch
Comment 3 Anders Carlsson 2014-06-18 17:12:02 PDT
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();
Comment 4 Roger Fong 2014-06-18 22:36:27 PDT
(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?
Comment 5 Roger Fong 2014-06-18 23:45:17 PDT
(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.,
Comment 6 Roger Fong 2014-06-19 00:06:46 PDT
Created attachment 233353 [details]
patch
Comment 7 Roger Fong 2014-06-19 00:39:14 PDT
Created attachment 233354 [details]
patch
Comment 8 Anders Carlsson 2014-06-19 11:55:33 PDT
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.
Comment 9 Roger Fong 2014-06-19 12:24:53 PDT
committed: http://trac.webkit.org/changeset/170156