Drop all file locks synchronously in network process when it is about to suspend

• Component: WebCore Misc.
• Status: NEW ---

[Sihui Liu]

...

[Sihui Liu]

Created attachment 433360 [details]
Patch

[Chris Dumez]

I don't "Drop file locks synchronously on PrepareToSuspend message" something we want. I wish there was better communication before implementing such things.

I start schedule a database write, home out of Safari, I want my database write to happen (before we suspend or after) but it should happen.

Aggressively aborting database operations on homing out seems very wrong to me. What's the point of us taking the process assertion then?

[Chris Dumez]

(In reply to Chris Dumez from comment #2)
> I don't "Drop file locks synchronously on PrepareToSuspend message"
> something we want. I wish there was better communication before implementing
> such things.
> 
> I start schedule a database write, home out of Safari, I want my database
> write to happen (before we suspend or after) but it should happen.
> 
> Aggressively aborting database operations on homing out seems very wrong to
> me. What's the point of us taking the process assertion then?

Geoff had a proposal on the other bug that seemed reasonable. I don't really understand how we ended up with an "abort everything on PrepareToSuspend" proposal.

[Sihui Liu]

(In reply to Chris Dumez from comment #3)
> (In reply to Chris Dumez from comment #2)
> > I don't "Drop file locks synchronously on PrepareToSuspend message"
> > something we want. I wish there was better communication before implementing
> > such things.
> > 
> > I start schedule a database write, home out of Safari, I want my database
> > write to happen (before we suspend or after) but it should happen.
> > 
> > Aggressively aborting database operations on homing out seems very wrong to
> > me. What's the point of us taking the process assertion then?
> 
> Geoff had a proposal on the other bug that seemed reasonable. I don't really
> understand how we ended up with an "abort everything on PrepareToSuspend"
> proposal.

This patch is more about providing a way to drop all database locks synchronously, which aims to solve crash of process being suspended before asynchronous suspend task. We may use it in imminent PrepareToSuspend or invalidation handler. I just put it in PrepareToSuspend because that's current IDB behavior: abort on PrepareToSuspend messages.

The other bug(https://bugs.webkit.org/show_bug.cgi?id=227778) is about not aborting things aggressively on non-imminent suspension. We may solve it for IDB by scheduling async task to suspend thread on non-imminent PrepareToSuspend like we did for LocalStorage. It's easier to do that if we can remove current IDB suspend code after this patch.

I am just uploading the initial patch for review and discussion. Will also need to check perf and tests results.

[Sihui Liu]

Created attachment 433372 [details]
Patch

[Sihui Liu]

Created attachment 433374 [details]
Patch

[Sihui Liu]

Created attachment 433381 [details]
Patch

[Sihui Liu]

Created attachment 433440 [details]
Patch

[Sihui Liu]

Created attachment 433442 [details]
Patch

[Sihui Liu]

Created attachment 433443 [details]
Patch

[Chris Dumez]

Comment on attachment 433443 [details]
Patch

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

Overall, this seems like a risky change to make at this point in the cycle. I don't know if we have evidence that this is still an issue after our previous fixes either.
I personally worry about the current behavior of IDB being way too aggressive and I wish we'd fix that before shipping, not make everything else like IDB.

> Source/WebKit/ChangeLog:9
> +        current implementation is IndexedDB releases locks by aborting transactions synchronously on PrepareToSuspend,

The current behavior of IndexedDB is bad and we know it's bad: we don't want to trigger an IndexedDB write, home out of Safari and have that IndexedDB write get lost.
As a result, aligning other better implementations with the bad IDB one is not the way to go IMO.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:-237
> -            SQLiteTransactionInProgressAutoCounter transactionCounter;

Dropping this seems like a regression. The database file may be locked during the closing of the database and this SQLiteTransactionInProgressAutoCounter was what was causing us to take a process assertion for the process. I get that your SQLiteLockScope below is going to keep it working for the network process. However, I don't see what keeps it working for database activity in the WebProcess (which relies on the SQLiteDatabaseTracker and setIsHoldingLockedFiles IPC).

> Source/WebCore/platform/sql/SQLiteDatabase.h:61
> +    WEBCORE_EXPORT bool open(const String& filename, OpenMode = OpenMode::ReadWriteCreate, IsInterruptable = IsInterruptable::No);

This new flag is super confusing to me. What does this mean? Why am given a choice? When do I want my database to be interruptible or not?

> Source/WebCore/platform/sql/SQLiteStatement.cpp:72
> +        SQLiteLockScope lockScope;

So now unrelated database operations (IndexedDB, local storage, ITP) which happen on different databases and on different threads have to be synchronized via locking?
I can't imagine this being great for perf and it feels odd since they are completely unrelated and should be able to proceed in parallel.

> Source/WebCore/platform/sql/SQLiteStatement.cpp:73
> +        error = sqlite3_step(m_statement);

So we used to take a process assertion from the duration of *transactions*, now we only take them for the duration to the statement stepping. Why is that better/ok? Isn't the database file locked during a transaction, no matter if we're stepping at the moment or not?

> Source/WebCore/platform/sql/SQLiteStatement.cpp:84
> +    Locker databaseLock { m_database.databaseMutex() };

Funny how this function says WithoutLock() and the first thing it does is locking. I guess you meant "without that other lock" since there is more than one lock now. Still looks weird.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2233
> -    WebResourceLoadStatisticsStore::suspend([callbackAggregator] { });

I strongly disagree with this change.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2250
> -    m_storageManagerSet->suspend([callbackAggregator] { });

ditto.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2239
> +    m_databaseAssertionScope.interrupt();

Is this "interruptAndSuspend"? ...

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2268
> +    m_databaseAssertionScope.resume();

... Since we're resuming here

> Source/WebKit/NetworkProcess/NetworkProcess.h:576
> +    DatabaseAssertionScope m_databaseAssertionScope;

It is super confusing to have a "scope" as a data member. It makes it look like we're holding a database assertion as long as the NetworkProcess object lives.

> Source/WebKit/Shared/DatabaseAssertionScope.cpp:57
> +    m_assertionReleaseTimer.stop();

I don't understand how this can be thread-safe. As far as I understand it, end() can get called on any database thread. m_assertionReleaseTimer is a RunLoop timer which fires on the main thread. Stopping and Starting this timer on a background thread while the main thread starts firing the timer (and thus is updating the internal state of the RunLoopTimer) seems like it would be an issue, no?

> Source/WebKit/Shared/DatabaseAssertionScope.cpp:66
> +        m_assertionReleaseTimer.startOneShot(1_s);

What prevents this timer from firing while in the middle of a database operation? It seems we only stop it when ending a database operation. What if:
1. I start a database operation
2. End a database operation which starts the timer
3. Start a database operation that takes over 1 second to complete

Wouldn't that timer fire on the main thread in the middle of the database operation?

It looks like assertionReleaseTimerFired() does grab a lock so it won't release the process assertion in the middle of the database operation at least. However, at soon as that database operation releases the lock, assertionReleaseTimerFired() will release the process assertion no matter what, even if other database operations have started, no?

> Source/WebKit/Shared/DatabaseAssertionScope.h:54
> +    WeakHashSet<WebCore::SQLiteDatabase> m_databases;

WeakHashSet seems very unsafe to me. I suspect you'd see assertion hits on iOS-wk2 debug.
Those SQLiteDatabase belong to many different threads (ITP thread, LocalStorage thread, IDB thread) and get constructed/destroyed on those threads. How can you safely iterate over those on any thread?

I guess you have locking in place that makes it safe but WeakPtr doesn't know that and its assertions will very likely hit in debug.

> Source/WebKit/Shared/DatabaseAssertionScope.h:55
> +    bool m_interrupted { false };

Style: Prefix for boolean variables please.

[Radar WebKit Bug Importer]

<rdar://problem/80802224>

[Sihui Liu]

Created attachment 434121 [details]
Patch

[Sihui Liu]

Created attachment 434123 [details]
Patch

[Sihui Liu]

Created attachment 434125 [details]
Patch

[Sihui Liu]

Created attachment 434131 [details]
Patch

[Sihui Liu]

Created attachment 434135 [details]
Patch