[webkit-reviews] review denied: [Bug 196519] [iOS] Web process gets suspended while holding locked database files : [Attachment 366618] 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 196519: [iOS] Web process gets suspended while holding locked database
files
https://bugs.webkit.org/show_bug.cgi?id=196519

Attachment 366618: Patch

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




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

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

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:
isDatabaseOpeningForbidden()

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

Seems wrong to call this without locking since it relies on
s_transactionInProgressCounter.

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

> Source/WebKit/WebProcess/WebProcess.cpp:1499
> +	   std::lock_guard<Lock>
lock(SQLiteDatabaseTracker::SQLiteDatabaseTrackerMutex());

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.