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

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




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

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

Almost perfect. One last round.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:83
> +{

I think we should lock in here so that callers do know have to worry about
locking and so that we do not need to expose the mutex.

> Source/WebCore/platform/sql/SQLiteDatabase.h:142
> +    WEBCORE_EXPORT static Lock& isDatabaseOpeningForbiddenMutex();

I do not think we need to expose this or have it in the header. It should just
be a static in the cpp file.

> Source/WebCore/platform/sql/SQLiteDatabaseTracker.cpp:60
> +    std::lock_guard<Lock> lock(transactionInProgressMutex);

Can you add such lock to hasTransactionInProgress() as well?

> Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp:57
> +    ASSERT(isMainThread());

ASSERT(RunLoop::isMain()); since we are at WebKit layer.

> Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp:61
> +	   m_hysteresis.stop();

Calling stop() will not synchronously call
WebSQLiteDatabaseTracker::hysteresisUpdated(), it will schedule a timer to call
it. But then, your destructor will finish running and m_hysteresis will be
destroyed. As a result you will not be sending the
WebProcessProxy::SetIsHoldingLockedFiles() IPC you should bee sending. I would
suggest calling hysteresisUpdated(HysteresisState::Stopped) directly.

> Source/WebKit/WebProcess/WebProcess.cpp:1466
> +	   std::lock_guard<Lock>
lock(SQLiteDatabase::isDatabaseOpeningForbiddenMutex());

No need with my proposal.

> Source/WebKit/WebProcess/WebProcess.cpp:1516
> +	   std::lock_guard<Lock>
lock(SQLiteDatabase::isDatabaseOpeningForbiddenMutex());

No need with my proposal.

> Source/WebKit/WebProcess/WebProcess.cpp:1592
> +	   std::lock_guard<Lock>
lock(SQLiteDatabase::isDatabaseOpeningForbiddenMutex());

No need with my proposal.

> Source/WebKit/WebProcess/WebProcess.h:502
> +    std::unique_ptr<WebSQLiteDatabaseTracker> m_webSQLiteDatabaseTracker;

WebSQLiteDatabaseTracker can probably be forward-declared now.