[webkit-reviews] review denied: [Bug 196519] [iOS] Web process gets suspended while holding locked database files : [Attachment 366559] 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 366559: Patch

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




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

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

> Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp:45
> +bool& isProcessSuspended()

Seems weird (layer violation) to have a class in WebCore know about process
suspension. Although, it is weird because if the process were suspended, it
would not be able to call this function in the first place, since its code
would not be running.

> Source/WebKit/WebProcess/WebProcess.cpp:1470
> +	   SQLiteDatabaseTracker::isProcessSuspended() = true;

I guess "isAboutToBeSuspended" would be a more accurate name.

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

So the lock protects both the transaction in progress and the
isProcessSuspended now? I am asking because you're locking here but not above.

> Source/WebKit/WebProcess/WebProcess.cpp:1499
> +	   SQLiteDatabaseTracker::isProcessSuspended() = true;

This is not how we usually set variables in WebKit, you'd want a regular
setter.

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

It does not read well to lock a mutex for "transactionInProgress" and then set
isProcessSuspended, not transactionInProgress.