Bug 196372 – Stop IDB transactions to release locked database files when network process is ready to suspend

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

Despite we take process assertion when there are SQLiteTransactions running, it expire after some time and network process can be suspended immediately. We need to make sure databases are closed and database files are unlocked at that time.

[Sihui Liu]

<rdar://problem/48930116>

[Sihui Liu]

Created attachment 366195 [details]
Patch

[Sihui Liu]

Created attachment 366210 [details]
Patch

[Brady Eidson]

Comment on attachment 366210 [details]
Patch

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

This seems fine.

I'm concerned about testing it - How did you do so?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:870
> +        m_suspendCompletionHandler = { };

m_suspendCompletionHandler = nullptr;

[Sihui Liu]

Created attachment 366438 [details]
Patch

[Sihui Liu]

Created attachment 366608 [details]
Patch

[Sihui Liu]

Created attachment 367001 [details]
Patch

[Brady Eidson]

Comment on attachment 367001 [details]
Patch

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

In general, the "hang the main thread of the NetworkProcess" seems bad.

Are NetworkProcess::cancelPrepareToSuspend and NetworkProcess:processDidResume basically the same? If so can we share code?

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:430
> +    Vector<SQLiteDatabase*> databasesCopy;
> +    {
> +        std::lock_guard<Lock> lock(allOpenDatabasesMutex);
> +        databasesCopy = copyToVector(allOpenDatabases());
> +    }

This can be moved to a utility method up front that takes the lock, makes the Vector, and returns it. That way no user of the map has to ever access the map directly or worry about the lock themselves.

[Chris Dumez]

Comment on attachment 367001 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Track all opened SQLiteDatases so we can interrput long-running transactions of them and make database close 

typo: interrput

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:864
> +    for (auto& database : openDatabases)

Is it guaranteed that the UniqueIDBDatabase objects are still alive as you iterate here?

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:130
> +    WEBCORE_EXPORT void closeAllDatabases();

We probably want a name that indicates that this is synchronous?

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:89
> +    static NeverDestroyed<HashSet<SQLiteDatabase*>> databases;

You should assert that the lock is held here.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:426
> +    Vector<SQLiteDatabase*> databasesCopy;

This looks extremely unsafe to me. Those SQLiteDatabase files may get destroyed on background threads as you iterate below it seems like.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:140
> +    , m_taskCounter([this](RefCounterEvent) { sendReadyToSuspendIfDone(); })

This name is way too generic considering what it does. This is very specific to suspension and I also find it a little bit odd to have it as a data member considering that is is only useful during suspension. I thought the previous code was better.

[Sihui Liu]

Created attachment 367939 [details]
Patch

[Sihui Liu]

Created attachment 367954 [details]
Patch

[Sihui Liu]

Created attachment 368002 [details]
Patch

[Chris Dumez]

Comment on attachment 368002 [details]
Patch

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

> Source/WebKit/ChangeLog:20
> [phone]  Sihui Liu  <[e-mail]>

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::SetIsIndexedDatabaseHoldingLockedFiles(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

[Sihui Liu]

Comment on attachment 368002 [details]
Patch

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

>> Source/WebKit/ChangeLog:20
>> [phone]  Sihui Liu  <[e-mail]>
> 
> Double change log.

Oops.

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

Okay, will change to suspendAndWait and use suspend.

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

Okay.

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

Okay, will change to setObserver.

>> Source/WebCore/Modules/indexeddb/server/IDBActivityCounter.cpp:44
>> +    countObserver = observer;
> 
> Should we assert that countObserver is nullptr before we set it?

We could.

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

This has a global count so it's named counter, but we can still make the name shorter as IDBActivityCounter::CounterObserver and  IDBActivityCounter::AutoCounter?

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

Will use AutoCounter as class name.

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

Okay.

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

ShouldForceStop::Yes means stop all ongoing transactions; ShouldForceStop::No means if there is ongoing transactions, do nothing and let the transactions run. Because counter not being 0 indicates the setIsIndexedDBHoldingLockedFiles message will or already be sent out.

Is tryStop() better?

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

Okay.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:247
>> +        IDBActivityCounter::IDBAcitivityAutoCounter autoCounter;
> 
> Wouldn't IDBActivity::AutoCounter look better?

How about IDBActivityCounter::AutoCounter?

>> 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?

Nice catch. Should decrement the counter before return if there is 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?

Yes it can fail to abort. We still decrement the count because errored abort or commit would be recognized as the end of the transaction and SQLiteIDBTransaction would be removed from map and gets destructed.

If we had problem in the future where we release the token early with unfinished transactions holding locked database file, we need to think of a way to force releasing file lock on sqlite.

>> 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?

Access to locked database file is happening on the background thread rather than the main thread; we probably want a more accurate counter.

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

Okay.

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

Okay.

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

Okay.

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

Okay.

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

Okay.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:140
>> +        parentProcessConnection()->send(Messages::NetworkProcessProxy::SetIsIndexedDatabaseHoldingLockedFiles(state == PAL::HysteresisState::Started), 0);
> 
> Why does IDB need special treatment? It is SQLLite based. So why cannot this rely on NetworkProcessProxy::SetIsHoldingLockedFiles ?

There are other SQLiteDatbases other than IDB in network process. IDBActivityCounter can only decide whether IDB needs locked file. When IDB doesn't need locked files it sends a "SetIsHoldingLockedFiles::No" message out but other databases could be holding locked files. So I felt it's better to let two counters have their own tokens.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1063
>> +void NetworkProcessProxy::setIsIndexedDatabaseHoldingLockedFiles(bool isIndexedDatabaseHoldingLockedFiles)
> 
> Why a whole separate handling for IDB???

Same as above.

[Sihui Liu]

Created attachment 368047 [details]
Patch

[Sihui Liu]

Created attachment 368128 [details]
Patch

[Chris Dumez]

Comment on attachment 368128 [details]
Patch

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

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:36
> +    SetIsIndexedDatabaseHoldingLockedFiles(bool isIndexedDatabaseHoldingLockedFiles)

I still do not understand why IDB needs its own file locking mechanism and why it does not rely and the normal SetIsHoldingLockedFiles().

[Chris Dumez]

Comment on attachment 368128 [details]
Patch

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

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:36
>> +    SetIsIndexedDatabaseHoldingLockedFiles(bool isIndexedDatabaseHoldingLockedFiles)
> 
> I still do not understand why IDB needs its own file locking mechanism and why it does not rely and the normal SetIsHoldingLockedFiles().

To reiterate, file locking happens at SQLite level, not at IDB level. We already have a SQLiteDatabaseTracker whose responsibility it is to know when SQLite is holding locked files and to notify the UIProcess in order to take the necessary assertions. If you have found an issue and the SQLiteDatabaseTracker fails to tell the UIProcess we're holding a locked file even though we are, please fix the issue in the SQLiteDatabaseTracker, not by adding a completely separate / duplicate mechanism specific to IDB.

[Sihui Liu]

Created attachment 368146 [details]
Patch

[Sihui Liu]

Created attachment 368148 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #18)
> Comment on attachment 368128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368128&action=review
> 
> >> Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:36
> >> +    SetIsIndexedDatabaseHoldingLockedFiles(bool isIndexedDatabaseHoldingLockedFiles)
> > 
> > I still do not understand why IDB needs its own file locking mechanism and why it does not rely and the normal SetIsHoldingLockedFiles().
> 
> To reiterate, file locking happens at SQLite level, not at IDB level. We
> already have a SQLiteDatabaseTracker whose responsibility it is to know when
> SQLite is holding locked files and to notify the UIProcess in order to take
> the necessary assertions. If you have found an issue and the
> SQLiteDatabaseTracker fails to tell the UIProcess we're holding a locked
> file even though we are, please fix the issue in the SQLiteDatabaseTracker,
> not by adding a completely separate / duplicate mechanism specific to IDB.

Read the code again and I see your point. Removed the IDB counter.

[Chris Dumez]

Comment on attachment 368148 [details]
Patch

Ok, this looks better to me now but I'd like someone familiar with IDB to review.

[Brady Eidson]

Comment on attachment 368148 [details]
Patch

Okay seems fine.

[WebKit Commit Bot]

Comment on attachment 368148 [details]
Patch

Clearing flags on attachment: 368148

Committed r244687: <https://trac.webkit.org/changeset/244687>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.