Bug 188029 – Speculative fix for StorageProcess crash

• Component: New Bugs
• Status: RESOLVED INVALID

[Alex Christensen]

Speculative fix for StorageProcess crash

[Alex Christensen]

Created attachment 345801 [details]
Patch

[Chris Dumez]

Comment on attachment 345801 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345801&action=review

I am not in favor of making such a change:
1. We do not know for sure this is related to threading.
2. AFAIK, UniqueIDBDatabaseConnection should not be dealt with from non-main thread, they are not even ThreadSafeRefCounted

If we suspect threading, I think we should look at call sites. Also, adding the isMainThread() assertions is a good idea (maybe even release assertions?).

> Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:87
> +    Lock m_databaseConnectionsLock;

What about IDBConnectionToClient::connectionToClientClosed() ? You're failing to lock there so the Lock is not useful.

[Chris Dumez]

Also, this does not look safe:
void IDBConnectionToClient::connectionToClientClosed()
{
    auto databaseConnections = m_databaseConnections;

    for (auto connection : databaseConnections) {
        if (m_databaseConnections.contains(connection))
            connection->connectionClosedFromClient();
    }

    m_databaseConnections.clear();
}

databaseConnections is a Vector of raw pointers. I am assuming we're copying m_databaseConnections to a vector before looping because m_databaseConnections may change while we iterate. If databaseConnection objects get destroyed while we iterate, we'll use a stale pointer and crash, despite us copying.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Also, this does not look safe:
> void IDBConnectionToClient::connectionToClientClosed()
> {
>     auto databaseConnections = m_databaseConnections;
> 
>     for (auto connection : databaseConnections) {
>         if (m_databaseConnections.contains(connection))
>             connection->connectionClosedFromClient();
>     }
> 
>     m_databaseConnections.clear();
> }
> 
> databaseConnections is a Vector of raw pointers. I am assuming we're copying
> m_databaseConnections to a vector before looping because
> m_databaseConnections may change while we iterate. If databaseConnection
> objects get destroyed while we iterate, we'll use a stale pointer and crash,
> despite us copying.

We may want:
auto databaseConnections = copyToVectorOf<RefPtr<UniqueIDBDatabaseConnection>>(m_databaseConnections);

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Also, this does not look safe:
> > void IDBConnectionToClient::connectionToClientClosed()
> > {
> >     auto databaseConnections = m_databaseConnections;
> > 
> >     for (auto connection : databaseConnections) {
> >         if (m_databaseConnections.contains(connection))
> >             connection->connectionClosedFromClient();
> >     }
> > 
> >     m_databaseConnections.clear();
> > }
> > 
> > databaseConnections is a Vector of raw pointers. I am assuming we're copying
> > m_databaseConnections to a vector before looping because
> > m_databaseConnections may change while we iterate. If databaseConnection
> > objects get destroyed while we iterate, we'll use a stale pointer and crash,
> > despite us copying.
> 
> We may want:
> auto databaseConnections =
> copyToVectorOf<RefPtr<UniqueIDBDatabaseConnection>>(m_databaseConnections);

Actually, it appears to be safe. The following check:
if (m_databaseConnections.contains(connection))

should make sure that the connection is still alive since the connection destructor would have removed it from m_databaseConnections.

[Sihui Liu]

StorageProcess does not exist now.