[webkit-reviews] review denied: [Bug 196372] Close open IDBDatabase when network process is ready to suspend : [Attachment 367001] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 196372: Close open IDBDatabase when network process is ready to suspend
https://bugs.webkit.org/show_bug.cgi?id=196372

Attachment 367001: Patch

https://bugs.webkit.org/attachment.cgi?id=367001&action=review




--- Comment #9 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 367001
  --> https://bugs.webkit.org/attachment.cgi?id=367001
Patch

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

> Source/WebCore/ChangeLog:9
> +	   Track all opened SQLiteDatases so we can interrput long-running
transactions of them and make database close 

typo: interrput

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:864
> +    for (auto& database : openDatabases)

Is it guaranteed that the UniqueIDBDatabase objects are still alive as you
iterate here?

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:130
> +    WEBCORE_EXPORT void closeAllDatabases();

We probably want a name that indicates that this is synchronous?

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:89
> +    static NeverDestroyed<HashSet<SQLiteDatabase*>> databases;

You should assert that the lock is held here.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:426
> +    Vector<SQLiteDatabase*> databasesCopy;

This looks extremely unsafe to me. Those SQLiteDatabase files may get destroyed
on background threads as you iterate below it seems like.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:140
> +    , m_taskCounter([this](RefCounterEvent) { sendReadyToSuspendIfDone(); })

This name is way too generic considering what it does. This is very specific to
suspension and I also find it a little bit odd to have it as a data member
considering that is is only useful during suspension. I thought the previous
code was better.