[webkit-reviews] review granted: [Bug 135218] IDB transactions never reset if the Web Process ends before cleaning up : [Attachment 235388] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jeffrey Pfau <jpfau at apple.com>'s
request for review:
Bug 135218: IDB transactions never reset if the Web Process ends before
cleaning up
https://bugs.webkit.org/show_bug.cgi?id=135218

Attachment 235388: Patch
https://bugs.webkit.org/attachment.cgi?id=235388&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235388&action=review


r=me but this has some really inefficient idioms that involve copying whole
hash tables multiple times, and could easily be made more efficient.

> Source/WebKit2/DatabaseProcess/DatabaseToWebProcessConnection.cpp:77
> -
> +    // The WebProcess has disconnected, close all of the connections
associated with it
> +    IDBConnectionMap idbConnections = m_idbConnections;
> +    for (auto serverConnectionIdentifier : idbConnections)
> +	   removeDatabaseProcessIDBConnection(serverConnectionIdentifier.key);

I suggest auto& rather than auto for the identifiers to avoid unnecessary
copying of each key/value pair. And iterating keys() so we can get at the key
without ".key()" each time.

But copying the entire map just to iterate the keys is not efficient. Instead,
if we can’t iterate the map in place, we should copy the keys into a vector
with copyKeysToVector and iterate the vector.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:720
> +	   HashSet<IDBIdentifier> transactions =
m_establishedTransactions.get(&transactionIdentifier.connection());
> +	   transactions.add(transactionIdentifier);
> +	   m_establishedTransactions.set(&transactionIdentifier.connection(),
transactions);

This can be done much more efficiently with add rather than get/set. Copying
the set out of the map and back into the map every time is not good for
performance, even if we added std::move to the set call to eliminate one of the
copies. Instead we should do an add, passing an empty set, which will find the
existing set. Then we call add on the set we found inside the map.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:755
> +	   HashSet<IDBIdentifier> transactions =
m_establishedTransactions.get(&transactionIdentifier.connection());
> +	   transactions.remove(transactionIdentifier);
> +	   m_establishedTransactions.set(&transactionIdentifier.connection(),
transactions);

Same comment as with add above.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1089
> +    if (!m_establishedTransactions.contains(&connection))
> +	   return;
> +
> +    HashSet<IDBIdentifier> transactions =
m_establishedTransactions.get(&connection);

Should not do a contains followed by an get, since that’s two hash table
lookups.

Also, if we can’t iterate the transactions in place inside the hash table, we
should copy out the identifiers into a vector rather than copying the set.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1091
> +    for (auto transactionIdentifier : transactions) {

I suggest auto& instead of auto.