Bug 188029

Created attachment 345801 [details] Patch

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.

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.

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

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

StorageProcess does not exist now.