Bug 134042 – Don't kill the UIProcess until all local storage transactions have been committed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Roger Fong]

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

[Roger Fong]

Created attachment 233329 [details]
patch

[Roger Fong]

Created attachment 233330 [details]
patch

[Anders Carlsson]

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();

[Roger Fong]

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

[Roger Fong]

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

[Roger Fong]

Created attachment 233353 [details]
patch

[Roger Fong]

Created attachment 233354 [details]
patch

[Anders Carlsson]

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.

[Roger Fong]

committed: http://trac.webkit.org/changeset/170156