[webkit-reviews] review denied: [Bug 196372] Stop IDB transactions to release locked database files when network process is ready to suspend : [Attachment 368002] 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: Stop IDB transactions to release locked database files when network
process is ready to suspend
https://bugs.webkit.org/show_bug.cgi?id=196372

Attachment 368002: Patch

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




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

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

> Source/WebKit/ChangeLog:20
> +2019-04-22  Sihui Liu  <sihui_liu at apple.com>

Double change log.

> Source/WTF/wtf/CrossThreadTaskHandler.h:47
> +    WTF_EXPORT_PRIVATE void waitForStop();

I do not think this name is right. It looks like to me this method is actually
stopping work, not just waiting. Maybe it should be name stopAndWait(),
similarly to pre-existing callOnMainThreadAndWait(). Or maybe suspendAndWait()
since there is a resume function. IF so, we should rename the members to use
"suspend" rather than "stop".

> Source/WebCore/Modules/indexeddb/server/IDBActivityCounter.cpp:39
> +static IDBActivityCounterObserver* countObserver = nullptr;

= nullptr is not needed since this is a static, it gets 0 initialized. Note
that you did not write "count = 0" above so you're already relying on this
behavior.

> Source/WebCore/Modules/indexeddb/server/IDBActivityCounter.cpp:41
> +void registerObserver(IDBActivityCounterObserver* observer)

I think this should be named setObserver() since you can only set 1 observer. I
would use register when you can register several.

> Source/WebCore/Modules/indexeddb/server/IDBActivityCounter.cpp:44
> +    countObserver = observer;

Should we assert that countObserver is nullptr before we set it?

> Source/WebCore/Modules/indexeddb/server/IDBActivityCounter.h:34
> +namespace IDBActivityCounter {

Maybe if we name this IDBActivity, we could rename the classes to shorter names
like "CounterObserver" and "AutoCounter".

> Source/WebCore/Modules/indexeddb/server/IDBActivityCounter.h:50
> +class IDBAcitivityAutoCounter {

Typo: IDBAcitivityAutoCounter

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:61
> +enum class ShouldForceStop { No, Yes };

enum class ShouldForceStop : bool { No, Yes };

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:129
> +    WEBCORE_EXPORT void stop(ShouldForceStop);

The name is unclear, the name is stop() and then there is a flag asking if we
should forceStop. What's the difference? This should be renamed to be clearer.
Maybe something like:
enum class StoppingPolicy { WaitForCompletion, Abort };

Cannot be sure of the naming since I am not sure I understood the behavior of
your code.

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.h:88
> +    bool hasTransaction(const IDBResourceIdentifier& identifier) { return
m_transactions.contains(identifier); }

const ?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:247
> +	   IDBActivityCounter::IDBAcitivityAutoCounter autoCounter;

Wouldn't IDBActivity::AutoCounter look better?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:936
> +    IDBActivityCounter::incrementIDBActivityCounter();

This method can return an error. Is it really OK to increment here even when we
end up with an error?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:981
> +    auto result = transaction->abort();

Can this fail to abort? If so, is it OK to decrement unconditionally below?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1009
> +    IDBActivityCounter::decrementIDBActivityCounter();

This looks a bit fragile. Would it look better if we moved transaction counting
to IDBTransaction?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:2707
> +bool SQLiteIDBBackingStore::hasTransaction(const IDBResourceIdentifier&
transactionIdentifier)

const ?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:1635
> +    if (transaction->state() ==
UniqueIDBDatabaseTransaction::State::Aborted)

We usually prefer to switch() to make sure we deal explicitly with all enum
values.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:2314
> +	   if (transaction->state() ==
UniqueIDBDatabaseTransaction::State::Aborting)

ditto about switch()

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.h:96
> +    State state() { return m_state; }

const

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.h:98
> +    IDBError result() { return m_result; }

Is IDBError a class/struct? IF so, this should return a const IDBError& and
this method would be const.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:140
> +	  
parentProcessConnection()->send(Messages::NetworkProcessProxy::SetIsIndexedData
baseHoldingLockedFiles(state == PAL::HysteresisState::Started), 0);

Why does IDB need special treatment? It is SQLLite based. So why cannot this
rely on NetworkProcessProxy::SetIsHoldingLockedFiles ?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1063
> +void NetworkProcessProxy::setIsIndexedDatabaseHoldingLockedFiles(bool
isIndexedDatabaseHoldingLockedFiles)

Why a whole separate handling for IDB???

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:263
> +    ProcessThrottler::BackgroundActivityToken
m_tokenForIndexedDatabaseHoldingLockedFiles;

Ditto