[webkit-reviews] review denied: [Bug 196519] [iOS] Web process gets suspended while holding locked database files : [Attachment 366618] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 3 20:52:19 PDT 2019

Chris Dumez <cdumez at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 196519: [iOS] Web process gets suspended while holding locked database

Attachment 366618: Patch


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

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

> Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp:38
> +static bool s_isAboutToSuspend = false;

We do not use prefixes for statics in WebKit. The code above is bad coding
style, let's not replicate it.

>> Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp:46
>> +bool isAboutToSuspend()
> Again, I still do not think WebCore should know anything about process
suspension. This is a WebKit2 concept and should stay at WebKit layer, not

I guess if you want to keep this layering, you can simply use a name that makes
sense in WebCore. Given what this boolean actually does, I would call it
something like:

> Source/WebKit/WebProcess/WebProcess.cpp:1469
> +    if (!SQLiteDatabaseTracker::hasTransactionInProgress()) {

Seems wrong to call this without locking since it relies on

Also, why are we doing this only if there are no transactions in progress?

> Source/WebKit/WebProcess/WebProcess.cpp:1499
> +	   std::lock_guard<Lock>

Seems aggressive to hold this lock while calling closeAllDatabases(). If
closeAllDatabases() were to lock the mutex, we would deadlock.

We have some unnecessary code duplication now, you close the database here and
in actualPrepareToSuspend(). However, actualPrepareToSuspend() gets called 2
lines below.

More information about the webkit-reviews mailing list