Bug 227778 – Do not abort ongoing IDB transaction synchronously on non-imminent PrepareToSuspend message

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 433092 [details]
Patch

[Sihui Liu]

Created attachment 433116 [details]
Patch

[Chris Dumez]

Comment on attachment 433116 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> -    for (auto& server : m_webIDBServers.values())

Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 433116 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > -    for (auto& server : m_webIDBServers.values())
> 
> Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.

To repeat what I said last week, I think we should schedule a task to suspend / hang the thread in PrepareToSuspend and abort any pending transaction when the assertion gets invalidated.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 433116 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > 
> > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > -    for (auto& server : m_webIDBServers.values())
> > 
> > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> 
> To repeat what I said last week, I think we should schedule a task to
> suspend / hang the thread in PrepareToSuspend and abort any pending
> transaction when the assertion gets invalidated.

We want to suspend as soon as possible after the app has been backgrounded. When the app is backgrounded, it sends us the PrepareToSuspend IPC and we should stop doing any additional work (e.g. new IDB transactions) at this point that may delay suspension (due to the network process taking a process assertion).

[Sihui Liu]

(In reply to Chris Dumez from comment #5)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 433116 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > > 
> > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > > -    for (auto& server : m_webIDBServers.values())
> > > 
> > > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> > 
> > To repeat what I said last week, I think we should schedule a task to
> > suspend / hang the thread in PrepareToSuspend and abort any pending
> > transaction when the assertion gets invalidated.
> 
> We want to suspend as soon as possible after the app has been backgrounded.
> When the app is backgrounded, it sends us the PrepareToSuspend IPC and we
> should stop doing any additional work (e.g. new IDB transactions) at this
> point that may delay suspension (due to the network process taking a process
> assertion).

With you proposed approach that network process receives PrepareToSuspend message and schedules a suspend task to IDB thread, there are 2 possible cases:

1. task runs before network process assertion is invalidated: thread is suspended in task and transactions are aborted in invalidation handler => why don't we just abort transactions in the task since nothing scheduled on the thread after the task can run after suspension? In this case, aborting earlier in the task also means network process can release the assertion sooner.

2. network process assertion is invalidated before task runs (maybe due to too many tasks scheduled on the thread), old transactions are aborted but thread is not suspended and new transaction can start => not fixing the crash

Both cases indicate that suspend the thread and abort transactions should happen in the same place. Either we do the preparation work in receiving prepareToSuspend message, or we do it in invalidation handler.

And this is how we suspend IDB: grab a lock to abort things on the main thread, and keep the lock until resume, which is synchronous, unlike the ITP or localstorage task.

[Chris Dumez]

(In reply to Sihui Liu from comment #6)
> (In reply to Chris Dumez from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > (In reply to Chris Dumez from comment #3)
> > > > Comment on attachment 433116 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > > > 
> > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > > > -    for (auto& server : m_webIDBServers.values())
> > > > 
> > > > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> > > 
> > > To repeat what I said last week, I think we should schedule a task to
> > > suspend / hang the thread in PrepareToSuspend and abort any pending
> > > transaction when the assertion gets invalidated.
> > 
> > We want to suspend as soon as possible after the app has been backgrounded.
> > When the app is backgrounded, it sends us the PrepareToSuspend IPC and we
> > should stop doing any additional work (e.g. new IDB transactions) at this
> > point that may delay suspension (due to the network process taking a process
> > assertion).
> 
> With you proposed approach that network process receives PrepareToSuspend
> message and schedules a suspend task to IDB thread, there are 2 possible
> cases:
> 
> 1. task runs before network process assertion is invalidated: thread is
> suspended in task and transactions are aborted in invalidation handler =>
> why don't we just abort transactions in the task since nothing scheduled on
> the thread after the task can run after suspension? In this case, aborting
> earlier in the task also means network process can release the assertion
> sooner.

We want to avoid data loss as much as possible. We should only abort transactions when we're really about the get suspended. PrepareToSuspend indicates we should get ready to get suspended and thus stop scheduling new work. However, the network process is holding an assertion for whatever current SQLiteTransaction to finish.
It is best to let this already running transaction finish rather than abort it and potentially lose data. The whole point of taking an assertion is to let an operation finish before suspension.

> 
> 2. network process assertion is invalidated before task runs (maybe due to
> too many tasks scheduled on the thread), old transactions are aborted but
> thread is not suspended and new transaction can start => not fixing the crash

Right, when the assertion is invalidated, we should abort all existing tasks and make sure the thread is suspended (synchronously).

[Sihui Liu]

(In reply to Chris Dumez from comment #7)
> (In reply to Sihui Liu from comment #6)
> > (In reply to Chris Dumez from comment #5)
> > > (In reply to Chris Dumez from comment #4)
> > > > (In reply to Chris Dumez from comment #3)
> > > > > Comment on attachment 433116 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > > > > 
> > > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > > > > -    for (auto& server : m_webIDBServers.values())
> > > > > 
> > > > > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> > > > 
> > > > To repeat what I said last week, I think we should schedule a task to
> > > > suspend / hang the thread in PrepareToSuspend and abort any pending
> > > > transaction when the assertion gets invalidated.
> > > 
> > > We want to suspend as soon as possible after the app has been backgrounded.
> > > When the app is backgrounded, it sends us the PrepareToSuspend IPC and we
> > > should stop doing any additional work (e.g. new IDB transactions) at this
> > > point that may delay suspension (due to the network process taking a process
> > > assertion).
> > 
> > With you proposed approach that network process receives PrepareToSuspend
> > message and schedules a suspend task to IDB thread, there are 2 possible
> > cases:
> > 
> > 1. task runs before network process assertion is invalidated: thread is
> > suspended in task and transactions are aborted in invalidation handler =>
> > why don't we just abort transactions in the task since nothing scheduled on
> > the thread after the task can run after suspension? In this case, aborting
> > earlier in the task also means network process can release the assertion
> > sooner.
> 
> We want to avoid data loss as much as possible. We should only abort
> transactions when we're really about the get suspended. PrepareToSuspend
> indicates we should get ready to get suspended and thus stop scheduling new
> work. However, the network process is holding an assertion for whatever
> current SQLiteTransaction to finish.
> It is best to let this already running transaction finish rather than abort
> it and potentially lose data. The whole point of taking an assertion is to
> let an operation finish before suspension.

IDB's transactions are interleaving, so the task queue can be:
1. start transaction-1 
2. execute transaction-1 statement
3. start transaction-2
4. commit transaction-1
5. execute transaction-2 statement 
6. commit transaction-2
And the tasks are dispatched from IPC thread.

The async suspend task by PrepareToSuspend can be at any position of the queue, and aborting transaction in the invalidation handler will happen during the suspend task (if invalidation handler is called before ProcessDidResume). So I am not sure how scheduling async suspend task help avoid data loss. Can you give an example?

> 
> > 
> > 2. network process assertion is invalidated before task runs (maybe due to
> > too many tasks scheduled on the thread), old transactions are aborted but
> > thread is not suspended and new transaction can start => not fixing the crash
> 
> Right, when the assertion is invalidated, we should abort all existing tasks
> and make sure the thread is suspended (synchronously).

[Chris Dumez]

(In reply to Sihui Liu from comment #8)
> (In reply to Chris Dumez from comment #7)
> > (In reply to Sihui Liu from comment #6)
> > > (In reply to Chris Dumez from comment #5)
> > > > (In reply to Chris Dumez from comment #4)
> > > > > (In reply to Chris Dumez from comment #3)
> > > > > > Comment on attachment 433116 [details]
> > > > > > Patch
> > > > > > 
> > > > > > View in context:
> > > > > > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > > > > > 
> > > > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > > > > > -    for (auto& server : m_webIDBServers.values())
> > > > > > 
> > > > > > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> > > > > 
> > > > > To repeat what I said last week, I think we should schedule a task to
> > > > > suspend / hang the thread in PrepareToSuspend and abort any pending
> > > > > transaction when the assertion gets invalidated.
> > > > 
> > > > We want to suspend as soon as possible after the app has been backgrounded.
> > > > When the app is backgrounded, it sends us the PrepareToSuspend IPC and we
> > > > should stop doing any additional work (e.g. new IDB transactions) at this
> > > > point that may delay suspension (due to the network process taking a process
> > > > assertion).
> > > 
> > > With you proposed approach that network process receives PrepareToSuspend
> > > message and schedules a suspend task to IDB thread, there are 2 possible
> > > cases:
> > > 
> > > 1. task runs before network process assertion is invalidated: thread is
> > > suspended in task and transactions are aborted in invalidation handler =>
> > > why don't we just abort transactions in the task since nothing scheduled on
> > > the thread after the task can run after suspension? In this case, aborting
> > > earlier in the task also means network process can release the assertion
> > > sooner.
> > 
> > We want to avoid data loss as much as possible. We should only abort
> > transactions when we're really about the get suspended. PrepareToSuspend
> > indicates we should get ready to get suspended and thus stop scheduling new
> > work. However, the network process is holding an assertion for whatever
> > current SQLiteTransaction to finish.
> > It is best to let this already running transaction finish rather than abort
> > it and potentially lose data. The whole point of taking an assertion is to
> > let an operation finish before suspension.
> 
> IDB's transactions are interleaving, so the task queue can be:
> 1. start transaction-1 
> 2. execute transaction-1 statement
> 3. start transaction-2
> 4. commit transaction-1
> 5. execute transaction-2 statement 
> 6. commit transaction-2
> And the tasks are dispatched from IPC thread.
> 
> The async suspend task by PrepareToSuspend can be at any position of the
> queue, and aborting transaction in the invalidation handler will happen
> during the suspend task (if invalidation handler is called before
> ProcessDidResume). So I am not sure how scheduling async suspend task help
> avoid data loss. Can you give an example?
> 
> > 
> > > 
> > > 2. network process assertion is invalidated before task runs (maybe due to
> > > too many tasks scheduled on the thread), old transactions are aborted but
> > > thread is not suspended and new transaction can start => not fixing the crash
> > 
> > Right, when the assertion is invalidated, we should abort all existing tasks
> > and make sure the thread is suspended (synchronously).

If PrepareToSuspend schedules a task to suspend/hang the thread, then no new transactions can be started from this point, they'll just get queued. Upon doing process suspension and then later resuming, those queued transactions can now get processes and won't get lost/aborted.

If we hadn't suspended/hung the threads then those transactions would have been started and could thus have been aborted when the process assertion gets invalidated.

I don't know much about IDB but I do know that it would be very unfortunate if we started *new* IDB transactions after receiving the PrepareToSuspend IPC. My understanding is that this was the reason why we suspended the IDB thread in PrepareToSuspend.

[Sihui Liu]

(In reply to Chris Dumez from comment #9)
> (In reply to Sihui Liu from comment #8)
> > (In reply to Chris Dumez from comment #7)
> > > (In reply to Sihui Liu from comment #6)
> > > > (In reply to Chris Dumez from comment #5)
> > > > > (In reply to Chris Dumez from comment #4)
> > > > > > (In reply to Chris Dumez from comment #3)
> > > > > > > Comment on attachment 433116 [details]
> > > > > > > Patch
> > > > > > > 
> > > > > > > View in context:
> > > > > > > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > > > > > > 
> > > > > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > > > > > > -    for (auto& server : m_webIDBServers.values())
> > > > > > > 
> > > > > > > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> > > > > > 
> > > > > > To repeat what I said last week, I think we should schedule a task to
> > > > > > suspend / hang the thread in PrepareToSuspend and abort any pending
> > > > > > transaction when the assertion gets invalidated.
> > > > > 
> > > > > We want to suspend as soon as possible after the app has been backgrounded.
> > > > > When the app is backgrounded, it sends us the PrepareToSuspend IPC and we
> > > > > should stop doing any additional work (e.g. new IDB transactions) at this
> > > > > point that may delay suspension (due to the network process taking a process
> > > > > assertion).
> > > > 
> > > > With you proposed approach that network process receives PrepareToSuspend
> > > > message and schedules a suspend task to IDB thread, there are 2 possible
> > > > cases:
> > > > 
> > > > 1. task runs before network process assertion is invalidated: thread is
> > > > suspended in task and transactions are aborted in invalidation handler =>
> > > > why don't we just abort transactions in the task since nothing scheduled on
> > > > the thread after the task can run after suspension? In this case, aborting
> > > > earlier in the task also means network process can release the assertion
> > > > sooner.
> > > 
> > > We want to avoid data loss as much as possible. We should only abort
> > > transactions when we're really about the get suspended. PrepareToSuspend
> > > indicates we should get ready to get suspended and thus stop scheduling new
> > > work. However, the network process is holding an assertion for whatever
> > > current SQLiteTransaction to finish.
> > > It is best to let this already running transaction finish rather than abort
> > > it and potentially lose data. The whole point of taking an assertion is to
> > > let an operation finish before suspension.
> > 
> > IDB's transactions are interleaving, so the task queue can be:
> > 1. start transaction-1 
> > 2. execute transaction-1 statement
> > 3. start transaction-2
> > 4. commit transaction-1
> > 5. execute transaction-2 statement 
> > 6. commit transaction-2
> > And the tasks are dispatched from IPC thread.
> > 
> > The async suspend task by PrepareToSuspend can be at any position of the
> > queue, and aborting transaction in the invalidation handler will happen
> > during the suspend task (if invalidation handler is called before
> > ProcessDidResume). So I am not sure how scheduling async suspend task help
> > avoid data loss. Can you give an example?
> > 
> > > 
> > > > 
> > > > 2. network process assertion is invalidated before task runs (maybe due to
> > > > too many tasks scheduled on the thread), old transactions are aborted but
> > > > thread is not suspended and new transaction can start => not fixing the crash
> > > 
> > > Right, when the assertion is invalidated, we should abort all existing tasks
> > > and make sure the thread is suspended (synchronously).
> 
> If PrepareToSuspend schedules a task to suspend/hang the thread, then no new
> transactions can be started from this point, they'll just get queued. Upon
> doing process suspension and then later resuming, those queued transactions
> can now get processes and won't get lost/aborted.

No task can be handled from the point: meaning no new transaction can be started and no ongoing transaction can be finished: we cannot commit ongoing IDB transaction because they may have statements to execute in later tasks.

> 
> If we hadn't suspended/hung the threads then those transactions would have
> been started and could thus have been aborted when the process assertion
> gets invalidated.
> 

It's also possible if we don't suspend the thread, all transactions (including the new ones) can be finished before timeout. There is no evidence or proof that suspending in PrepareToSuspend will give us better chance of no rollback.

> I don't know much about IDB but I do know that it would be very unfortunate
> if we started *new* IDB transactions after receiving the PrepareToSuspend
> IPC. My understanding is that this was the reason why we suspended the IDB
> thread in PrepareToSuspend.

We suspended the IDB thread (and aborted transactions) in PrepareToSuspend, because that's only indicator that network process is going to be suspended. We have to do that there. Now the indicator is network process's own assertion.

My understanding is UI process now sends PrepareToSuspend without considering the network process's SQLite activities, which it used to take care, so this patch just gets us close to the old behavior. We suspend IDB (suspending thread and aborting transaction) synchronously on the final notice. 

I tend to believe that's what we should do for other databases too. Either they don't suspend, or they suspend immediately/synchronously. That makes it easier to manage the suspension states and get things right (crashes show some thread is suspended and some thread is still processing tasks).

[Chris Dumez]

(In reply to Sihui Liu from comment #10)
> (In reply to Chris Dumez from comment #9)
> > (In reply to Sihui Liu from comment #8)
> > > (In reply to Chris Dumez from comment #7)
> > > > (In reply to Sihui Liu from comment #6)
> > > > > (In reply to Chris Dumez from comment #5)
> > > > > > (In reply to Chris Dumez from comment #4)
> > > > > > > (In reply to Chris Dumez from comment #3)
> > > > > > > > Comment on attachment 433116 [details]
> > > > > > > > Patch
> > > > > > > > 
> > > > > > > > View in context:
> > > > > > > > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > > > > > > > 
> > > > > > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > > > > > > > -    for (auto& server : m_webIDBServers.values())
> > > > > > > > 
> > > > > > > > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> > > > > > > 
> > > > > > > To repeat what I said last week, I think we should schedule a task to
> > > > > > > suspend / hang the thread in PrepareToSuspend and abort any pending
> > > > > > > transaction when the assertion gets invalidated.
> > > > > > 
> > > > > > We want to suspend as soon as possible after the app has been backgrounded.
> > > > > > When the app is backgrounded, it sends us the PrepareToSuspend IPC and we
> > > > > > should stop doing any additional work (e.g. new IDB transactions) at this
> > > > > > point that may delay suspension (due to the network process taking a process
> > > > > > assertion).
> > > > > 
> > > > > With you proposed approach that network process receives PrepareToSuspend
> > > > > message and schedules a suspend task to IDB thread, there are 2 possible
> > > > > cases:
> > > > > 
> > > > > 1. task runs before network process assertion is invalidated: thread is
> > > > > suspended in task and transactions are aborted in invalidation handler =>
> > > > > why don't we just abort transactions in the task since nothing scheduled on
> > > > > the thread after the task can run after suspension? In this case, aborting
> > > > > earlier in the task also means network process can release the assertion
> > > > > sooner.
> > > > 
> > > > We want to avoid data loss as much as possible. We should only abort
> > > > transactions when we're really about the get suspended. PrepareToSuspend
> > > > indicates we should get ready to get suspended and thus stop scheduling new
> > > > work. However, the network process is holding an assertion for whatever
> > > > current SQLiteTransaction to finish.
> > > > It is best to let this already running transaction finish rather than abort
> > > > it and potentially lose data. The whole point of taking an assertion is to
> > > > let an operation finish before suspension.
> > > 
> > > IDB's transactions are interleaving, so the task queue can be:
> > > 1. start transaction-1 
> > > 2. execute transaction-1 statement
> > > 3. start transaction-2
> > > 4. commit transaction-1
> > > 5. execute transaction-2 statement 
> > > 6. commit transaction-2
> > > And the tasks are dispatched from IPC thread.
> > > 
> > > The async suspend task by PrepareToSuspend can be at any position of the
> > > queue, and aborting transaction in the invalidation handler will happen
> > > during the suspend task (if invalidation handler is called before
> > > ProcessDidResume). So I am not sure how scheduling async suspend task help
> > > avoid data loss. Can you give an example?
> > > 
> > > > 
> > > > > 
> > > > > 2. network process assertion is invalidated before task runs (maybe due to
> > > > > too many tasks scheduled on the thread), old transactions are aborted but
> > > > > thread is not suspended and new transaction can start => not fixing the crash
> > > > 
> > > > Right, when the assertion is invalidated, we should abort all existing tasks
> > > > and make sure the thread is suspended (synchronously).
> > 
> > If PrepareToSuspend schedules a task to suspend/hang the thread, then no new
> > transactions can be started from this point, they'll just get queued. Upon
> > doing process suspension and then later resuming, those queued transactions
> > can now get processes and won't get lost/aborted.
> 
> No task can be handled from the point: meaning no new transaction can be
> started and no ongoing transaction can be finished: we cannot commit ongoing
> IDB transaction because they may have statements to execute in later tasks.
> 
> > 
> > If we hadn't suspended/hung the threads then those transactions would have
> > been started and could thus have been aborted when the process assertion
> > gets invalidated.
> > 
> 
> It's also possible if we don't suspend the thread, all transactions
> (including the new ones) can be finished before timeout. There is no
> evidence or proof that suspending in PrepareToSuspend will give us better
> chance of no rollback.
> 
> > I don't know much about IDB but I do know that it would be very unfortunate
> > if we started *new* IDB transactions after receiving the PrepareToSuspend
> > IPC. My understanding is that this was the reason why we suspended the IDB
> > thread in PrepareToSuspend.
> 
> We suspended the IDB thread (and aborted transactions) in PrepareToSuspend,
> because that's only indicator that network process is going to be suspended.
> We have to do that there. Now the indicator is network process's own
> assertion.
> 
> My understanding is UI process now sends PrepareToSuspend without
> considering the network process's SQLite activities, which it used to take
> care, so this patch just gets us close to the old behavior. We suspend IDB
> (suspending thread and aborting transaction) synchronously on the final
> notice. 
> 
> I tend to believe that's what we should do for other databases too. Either
> they don't suspend, or they suspend immediately/synchronously. That makes it
> easier to manage the suspension states and get things right (crashes show
> some thread is suspended and some thread is still processing tasks).

Let me try another way: there is zero guarantee your invalidation handler will have time to run before suspension. Not only is there no guarantee, we see suspension happening before the invalidation handler getting called happening very frequently in logs.

[Sihui Liu]

(In reply to Chris Dumez from comment #11)
> (In reply to Sihui Liu from comment #10)
> > (In reply to Chris Dumez from comment #9)
> > > (In reply to Sihui Liu from comment #8)
> > > > (In reply to Chris Dumez from comment #7)
> > > > > (In reply to Sihui Liu from comment #6)
> > > > > > (In reply to Chris Dumez from comment #5)
> > > > > > > (In reply to Chris Dumez from comment #4)
> > > > > > > > (In reply to Chris Dumez from comment #3)
> > > > > > > > > Comment on attachment 433116 [details]
> > > > > > > > > Patch
> > > > > > > > > 
> > > > > > > > > View in context:
> > > > > > > > > https://bugs.webkit.org/attachment.cgi?id=433116&action=review
> > > > > > > > > 
> > > > > > > > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-2219
> > > > > > > > > > -    for (auto& server : m_webIDBServers.values())
> > > > > > > > > 
> > > > > > > > > Not suspending the IDB thread in PrepareToSuspend seems like a big no-no.
> > > > > > > > 
> > > > > > > > To repeat what I said last week, I think we should schedule a task to
> > > > > > > > suspend / hang the thread in PrepareToSuspend and abort any pending
> > > > > > > > transaction when the assertion gets invalidated.
> > > > > > > 
> > > > > > > We want to suspend as soon as possible after the app has been backgrounded.
> > > > > > > When the app is backgrounded, it sends us the PrepareToSuspend IPC and we
> > > > > > > should stop doing any additional work (e.g. new IDB transactions) at this
> > > > > > > point that may delay suspension (due to the network process taking a process
> > > > > > > assertion).
> > > > > > 
> > > > > > With you proposed approach that network process receives PrepareToSuspend
> > > > > > message and schedules a suspend task to IDB thread, there are 2 possible
> > > > > > cases:
> > > > > > 
> > > > > > 1. task runs before network process assertion is invalidated: thread is
> > > > > > suspended in task and transactions are aborted in invalidation handler =>
> > > > > > why don't we just abort transactions in the task since nothing scheduled on
> > > > > > the thread after the task can run after suspension? In this case, aborting
> > > > > > earlier in the task also means network process can release the assertion
> > > > > > sooner.
> > > > > 
> > > > > We want to avoid data loss as much as possible. We should only abort
> > > > > transactions when we're really about the get suspended. PrepareToSuspend
> > > > > indicates we should get ready to get suspended and thus stop scheduling new
> > > > > work. However, the network process is holding an assertion for whatever
> > > > > current SQLiteTransaction to finish.
> > > > > It is best to let this already running transaction finish rather than abort
> > > > > it and potentially lose data. The whole point of taking an assertion is to
> > > > > let an operation finish before suspension.
> > > > 
> > > > IDB's transactions are interleaving, so the task queue can be:
> > > > 1. start transaction-1 
> > > > 2. execute transaction-1 statement
> > > > 3. start transaction-2
> > > > 4. commit transaction-1
> > > > 5. execute transaction-2 statement 
> > > > 6. commit transaction-2
> > > > And the tasks are dispatched from IPC thread.
> > > > 
> > > > The async suspend task by PrepareToSuspend can be at any position of the
> > > > queue, and aborting transaction in the invalidation handler will happen
> > > > during the suspend task (if invalidation handler is called before
> > > > ProcessDidResume). So I am not sure how scheduling async suspend task help
> > > > avoid data loss. Can you give an example?
> > > > 
> > > > > 
> > > > > > 
> > > > > > 2. network process assertion is invalidated before task runs (maybe due to
> > > > > > too many tasks scheduled on the thread), old transactions are aborted but
> > > > > > thread is not suspended and new transaction can start => not fixing the crash
> > > > > 
> > > > > Right, when the assertion is invalidated, we should abort all existing tasks
> > > > > and make sure the thread is suspended (synchronously).
> > > 
> > > If PrepareToSuspend schedules a task to suspend/hang the thread, then no new
> > > transactions can be started from this point, they'll just get queued. Upon
> > > doing process suspension and then later resuming, those queued transactions
> > > can now get processes and won't get lost/aborted.
> > 
> > No task can be handled from the point: meaning no new transaction can be
> > started and no ongoing transaction can be finished: we cannot commit ongoing
> > IDB transaction because they may have statements to execute in later tasks.
> > 
> > > 
> > > If we hadn't suspended/hung the threads then those transactions would have
> > > been started and could thus have been aborted when the process assertion
> > > gets invalidated.
> > > 
> > 
> > It's also possible if we don't suspend the thread, all transactions
> > (including the new ones) can be finished before timeout. There is no
> > evidence or proof that suspending in PrepareToSuspend will give us better
> > chance of no rollback.
> > 
> > > I don't know much about IDB but I do know that it would be very unfortunate
> > > if we started *new* IDB transactions after receiving the PrepareToSuspend
> > > IPC. My understanding is that this was the reason why we suspended the IDB
> > > thread in PrepareToSuspend.
> > 
> > We suspended the IDB thread (and aborted transactions) in PrepareToSuspend,
> > because that's only indicator that network process is going to be suspended.
> > We have to do that there. Now the indicator is network process's own
> > assertion.
> > 
> > My understanding is UI process now sends PrepareToSuspend without
> > considering the network process's SQLite activities, which it used to take
> > care, so this patch just gets us close to the old behavior. We suspend IDB
> > (suspending thread and aborting transaction) synchronously on the final
> > notice. 
> > 
> > I tend to believe that's what we should do for other databases too. Either
> > they don't suspend, or they suspend immediately/synchronously. That makes it
> > easier to manage the suspension states and get things right (crashes show
> > some thread is suspended and some thread is still processing tasks).
> 
> Let me try another way: there is zero guarantee your invalidation handler
> will have time to run before suspension. Not only is there no guarantee, we
> see suspension happening before the invalidation handler getting called
> happening very frequently in logs.

Not sure if it matters but this is willinvalidate handler, not the invalidation handler; invalidation handler seems to be called after assertion is dropped?

If we don't think the handler will be called or it's very likely it's will not be called, then the proposal will also not work. We suspend the thread, but we don't abort transactions so database lock is still held. In this case, we'd better keep current implementation. 

But isn't our implementation based on that we will get the call? Compared to imminent prepareToSuspend, at least this message is sent directly to network process.

[Geoffrey Garen]

It seems like the problem in trunk is that, often, when we background Safari, we immediately receive prepareToSuspend(), and then respond by aborting in-flight IDB transactions.

Ideally, in prepareToSuspend(), we would (a) complete in-flight IDB transactions (using an assertion) and (b) avoid starting new IDB transactions, and (c) only abort transactions if they truly are super-long-running.

I think trunk does (a), this patch does (c); and doing (b) is a little challenging in our current architecture, because our current architecture doesn't distinguish new IDB transactions from operations on existing IDB transactions.

Is that right?

If so, doing only (c) now might be an improvement -- and we probably want to follow up and do (b) too.

Is that right?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #13)
> It seems like the problem in trunk is that, often, when we background
> Safari, we immediately receive prepareToSuspend(), and then respond by
> aborting in-flight IDB transactions.
> 
> Ideally, in prepareToSuspend(), we would (a) complete in-flight IDB
> transactions (using an assertion) and (b) avoid starting new IDB
> transactions, and (c) only abort transactions if they truly are
> super-long-running.
> 
> I think trunk does (a), this patch does (c); and doing (b) is a little
> challenging in our current architecture, because our current architecture
> doesn't distinguish new IDB transactions from operations on existing IDB
> transactions.
> 
> Is that right?
> 
> If so, doing only (c) now might be an improvement -- and we probably want to
> follow up and do (b) too.
> 
> Is that right?

That patch *attempts to* do (c). With this patch we would only have handling when the process assertion gets invalidated. As stated before, we already know for a fact that our invalidation logic doesn't reliably get called before suspension. This is *especially true* in the cases where we just keep scheduling work *after* the app was backgrounded (i.e. PrepareToSuspend has been received) because the 30 second timer may be close to exhausted when trying to take an assertion.

This patch makes things work by allowing work to get scheduled after PrepareToSuspend has been received, which we know makes it way more likely that:
1. Our assertion will get invalidated and our invalidation logic may or may not run
2. We run for 30 seconds after backgrounding, which will be a power regression.

Doing thing in an invalidation handler is a good idea but it should really be LAST resort, not our MAIN way of dealing with suspension. Our process assertion getting invalidated is a BUG, it should not be happening if we did our job right.

[Geoffrey Garen]

I see: There's a tradeoff. Making this change now would reduce the number of unwanted immediate IDB transaction aborts, reducing data loss; but it would also increase the number of suspensions while holding locked files, increasing Network process crashes (which, when they happen, lose the same data, and more).

Argh.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #15)
> I see: There's a tradeoff. Making this change now would reduce the number of
> unwanted immediate IDB transaction aborts, reducing data loss; but it would
> also increase the number of suspensions while holding locked files,
> increasing Network process crashes (which, when they happen, lose the same
> data, and more).
> 
> Argh.

That's why, while I think it is good to abort things in the invalidation handler, I still think we should do *something* in the PrepareToSuspend. I am not super familiar with IndexedDB so it is a bit difficult for me to know what that something can be but ideally:
1. We would stop scheduling new work / transactions
2. We would allow any open transaction to finish/commit (asynchronously). Hopefully, these will finish within the 30s invalidation. If they don't, the safety net (our invalidation handler) would trigger and abort them.

[Geoffrey Garen]

Maybe a reasonable compromise for the time being would be for prepareToSuspend (when not imminent) to schedule a 10s timer, and only abort transactions after that? 10s is well below the effective 30s limit, and I expect it to be well above the 95th percentile transaction time. (Need a little logic to only fire that timer while suspending, and not after resuming.)

Then we can make this incremental improvement now; and after we have a way to avoid starting new transactions, we can remove the abort behavior from prepareToSuspend entirely.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #17)
> Maybe a reasonable compromise for the time being would be for
> prepareToSuspend (when not imminent) to schedule a 10s timer, and only abort
> transactions after that? 10s is well below the effective 30s limit, and I
> expect it to be well above the 95th percentile transaction time. (Need a
> little logic to only fire that timer while suspending, and not after
> resuming.)
> 
> Then we can make this incremental improvement now; and after we have a way
> to avoid starting new transactions, we can remove the abort behavior from
> prepareToSuspend entirely.

Yes, something like this may be a reasonable compromise.

Although, if you don't prevent any new work from getting scheduled, are you really less likely to abort DB work if you wait 10 seconds before aborting?

[Geoffrey Garen]

After backgrounding, there's no more user interaction. Maybe that reduces the chances of new DB transactions.

Can we additionally count on WebContent process suspension preventing new DB transactions in the common case?

Relatedly, do we do anything in the WebContent process to prevent suspension during a DB transaction? (If we don't, we probably haven't solved the problem of aborting transactions, since WebContent can start a transaction and never stop it.)

[Chris Dumez]

(In reply to Geoffrey Garen from comment #19)
> After backgrounding, there's no more user interaction. Maybe that reduces
> the chances of new DB transactions.

User interaction may be a trigger for indexed db writes but as long as JS is running in the WebContent, I think it is safe to assume indexed db write may happen, user interactions or not.

> 
> Can we additionally count on WebContent process suspension preventing new DB
> transactions in the common case?

Yes, to some extent. *If* the WebProcess suspends promptly upon app backgrounding then it cannot schedule to IndexedDB work with the network process so this likely helps. Note the *if* though :) If there is something in the WebProcess that delays suspension (e.g. App Cache writing, loading, ...) then it could in theory keep scheduling work with the network process for up to 30 seconds.

> Relatedly, do we do anything in the WebContent process to prevent suspension
> during a DB transaction? (If we don't, we probably haven't solved the
> problem of aborting transactions, since WebContent can start a transaction
> and never stop it.)

No, I am not aware of any logic to prevent WebProcess suspension during an IDB transaction.

[Sihui Liu]

Chris suggested maybe we should make the change after (In reply to Geoffrey Garen from comment #19)
> After backgrounding, there's no more user interaction. Maybe that reduces
> the chances of new DB transactions.
> 

Nope, like Chris said.

> Can we additionally count on WebContent process suspension preventing new DB
> transactions in the common case?
> 

If all web processes using IDB are suspended, no new task will be scheduled, so no new transaction after all existing tasks. But we don't know when web process are suspended; they can also ask UI process to take background assertion.

> Relatedly, do we do anything in the WebContent process to prevent suspension
> during a DB transaction? (If we don't, we probably haven't solved the
> problem of aborting transactions, since WebContent can start a transaction
> and never stop it.)

Yes, we avoid aborting transaction (to avoid data loss) on a best-effort basis.
If user chooses to use API in this way, then it is expected that the transaction will likely never succeed.

The goal of this patch is to approach the old behavior at when we take the assertion in UI process for database activities. In previous implementation, if assertion is taken before app enters background, the assertion can last until timeout. We did not do anything to stop starting new transaction after backgrounding. To get the old behavior, the patch should be:
1. if PrepareToSuspend is imminent, suspend IDB (suspending thread + aborting transactions)
2. if PrepareToSuspend is non-imminent: 
2.1 if assertion is taken, let it run as long as timeout, and suspend IDB in assertion invalidation handler or when assertion is released
2.2 if assertion is not taken, suspend IDB

Our trunk behavior is: if PrepareToSuspend, suspend IDB. This can be a regression for avoiding data loss, e.g. in the case where app is backgrounded and foregrounded soon, where transactions would not be aborted before. But this can also be good for power, as we don't allow network process to stay active once backgrounded.

[Sihui Liu]

Created attachment 433535 [details]
Patch

[Sihui Liu]

Created attachment 433537 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/80602557>

[Sihui Liu]

Created attachment 433542 [details]
Patch

[Chris Dumez]

Comment on attachment 433542 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:776
> +        return true;

Shouldn't this be false?

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:55
> +    enum class IsSuspensionImminent : bool { No, Yes };

This should probably be in database terms, not suspension terms. IDB shouldn't really have to care about suspension. Should be something like: "ShouldAbortTransactions" or "ForceSuspension".

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:56
> +    bool suspend(IsSuspensionImminent = IsSuspensionImminent::Yes);

I think having a default parameter value is confusing when I look at call sites.

Maybe we should name this something like trySuspend() since it return a boolean and may not suspend?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2216
> +    m_shouldSuspendIDBServer = true;

m_shouldSuspendIDBServers (plural) ?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2219
> +        for (auto& server : m_webIDBServers.values())

Ideally, we would not duplicate the loop here and below for the non-imminent case.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2237
> +        if (!m_shouldSuspendIDBServer)

I guess we're not too concerned about this timer firing early in the case where you:
1. Home out of safari
2. quickly home back into Safari
3. Quickly home back out (you won't get 5 seconds)

It is kind of an edge case and the result is not terrible (we abort transactions early) so I guess this is fine.

> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:117
> +        callOnMainRunLoopAndWait([this, weakThis = WTFMove(weakThis)]() {

Why the "AndWait" ? I know if is important for an invalidation handler but is it useful for a prepare to invalidate handler? I don't know enough about this RB delegate to answer that.

> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:122
> +                server->suspend();

This will almost certainly introduce cases where the network process has resumed but its IDB threads are suspended. Imagine the network process getting a ProcessDidResume IPC in between your prepare for invalidation handler getting called on the background thread and you suspending IDBservers on the main thread.

Also note that since you're using a "prepareForInvalidationHandler" and not an invalidation handler, the assertion is still valid after you suspend IDB and may never actually get invalidated.

> Source/WebKit/UIProcess/ProcessAssertion.h:75
> +    void setPrepareForInvalidationHandler(Function<void()>&& handler) { m_prepareForInvalidationHandler = WTFMove(handler); }

Can't you use setInvalidationHandler() like the others? I know very little about the prepareForInvalidationCallback so it is hard for me to understand why we need this one instead of the invalidation handler that the rest of our code relies on.
May be worth discussing with RunningBoard developers to see which one is best and why.

[Sihui Liu]

Comment on attachment 433542 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:776
>> +        return true;
> 
> Shouldn't this be false?

We don't need to hang the thread for ephemeral session, but I guess it doesn't hurt to hang it; will change

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:55
>> +    enum class IsSuspensionImminent : bool { No, Yes };
> 
> This should probably be in database terms, not suspension terms. IDB shouldn't really have to care about suspension. Should be something like: "ShouldAbortTransactions" or "ForceSuspension".

Okay, will rename it.

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:56
>> +    bool suspend(IsSuspensionImminent = IsSuspensionImminent::Yes);
> 
> I think having a default parameter value is confusing when I look at call sites.
> 
> Maybe we should name this something like trySuspend() since it return a boolean and may not suspend?

Okay, will rename it to trySuspend.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2216
>> +    m_shouldSuspendIDBServer = true;
> 
> m_shouldSuspendIDBServers (plural) ?

Will change.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2219
>> +        for (auto& server : m_webIDBServers.values())
> 
> Ideally, we would not duplicate the loop here and below for the non-imminent case.

Like adding a function?

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2237
>> +        if (!m_shouldSuspendIDBServer)
> 
> I guess we're not too concerned about this timer firing early in the case where you:
> 1. Home out of safari
> 2. quickly home back into Safari
> 3. Quickly home back out (you won't get 5 seconds)
> 
> It is kind of an edge case and the result is not terrible (we abort transactions early) so I guess this is fine.

Yes; seems we don't need to dispatch a task again if there is a pending one though

>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:117
>> +        callOnMainRunLoopAndWait([this, weakThis = WTFMove(weakThis)]() {
> 
> Why the "AndWait" ? I know if is important for an invalidation handler but is it useful for a prepare to invalidate handler? I don't know enough about this RB delegate to answer that.

Just confirmed it's ~5s before invalidation. We don't need to AndWait I guess.

>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:122
>> +                server->suspend();
> 
> This will almost certainly introduce cases where the network process has resumed but its IDB threads are suspended. Imagine the network process getting a ProcessDidResume IPC in between your prepare for invalidation handler getting called on the background thread and you suspending IDBservers on the main thread.
> 
> Also note that since you're using a "prepareForInvalidationHandler" and not an invalidation handler, the assertion is still valid after you suspend IDB and may never actually get invalidated.

Yes, should check m_shouldSuspendIDBServer

>> Source/WebKit/UIProcess/ProcessAssertion.h:75
>> +    void setPrepareForInvalidationHandler(Function<void()>&& handler) { m_prepareForInvalidationHandler = WTFMove(handler); }
> 
> Can't you use setInvalidationHandler() like the others? I know very little about the prepareForInvalidationCallback so it is hard for me to understand why we need this one instead of the invalidation handler that the rest of our code relies on.
> May be worth discussing with RunningBoard developers to see which one is best and why.

I asked RunningBoard people: 
- (void)assertion:(RBSAssertion *)assertion didInvalidateWithError:(nullable NSError *)error can be called the same time as suspension happens, which means process can get suspended before the function returns.

[Chris Dumez]

(In reply to Sihui Liu from comment #27)
> Comment on attachment 433542 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433542&action=review
> 
> >> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:776
> >> +        return true;
> > 
> > Shouldn't this be false?
> 
> We don't need to hang the thread for ephemeral session, but I guess it
> doesn't hurt to hang it; will change
> 
> >> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:55
> >> +    enum class IsSuspensionImminent : bool { No, Yes };
> > 
> > This should probably be in database terms, not suspension terms. IDB shouldn't really have to care about suspension. Should be something like: "ShouldAbortTransactions" or "ForceSuspension".
> 
> Okay, will rename it.
> 
> >> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:56
> >> +    bool suspend(IsSuspensionImminent = IsSuspensionImminent::Yes);
> > 
> > I think having a default parameter value is confusing when I look at call sites.
> > 
> > Maybe we should name this something like trySuspend() since it return a boolean and may not suspend?
> 
> Okay, will rename it to trySuspend.
> 
> >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2216
> >> +    m_shouldSuspendIDBServer = true;
> > 
> > m_shouldSuspendIDBServers (plural) ?
> 
> Will change.
> 
> >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2219
> >> +        for (auto& server : m_webIDBServers.values())
> > 
> > Ideally, we would not duplicate the loop here and below for the non-imminent case.
> 
> Like adding a function?

No, just use the same loop:
for (auto& server : m_webIDBServers.values())
    server->trySuspend(isImminentSuspension? ForceSuspend::Yes : ForceSuspend::No);

> 
> >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2237
> >> +        if (!m_shouldSuspendIDBServer)
> > 
> > I guess we're not too concerned about this timer firing early in the case where you:
> > 1. Home out of safari
> > 2. quickly home back into Safari
> > 3. Quickly home back out (you won't get 5 seconds)
> > 
> > It is kind of an edge case and the result is not terrible (we abort transactions early) so I guess this is fine.
> 
> Yes; seems we don't need to dispatch a task again if there is a pending one
> though
> 
> >> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:117
> >> +        callOnMainRunLoopAndWait([this, weakThis = WTFMove(weakThis)]() {
> > 
> > Why the "AndWait" ? I know if is important for an invalidation handler but is it useful for a prepare to invalidate handler? I don't know enough about this RB delegate to answer that.
> 
> Just confirmed it's ~5s before invalidation. We don't need to AndWait I
> guess.
> 
> >> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:122
> >> +                server->suspend();
> > 
> > This will almost certainly introduce cases where the network process has resumed but its IDB threads are suspended. Imagine the network process getting a ProcessDidResume IPC in between your prepare for invalidation handler getting called on the background thread and you suspending IDBservers on the main thread.
> > 
> > Also note that since you're using a "prepareForInvalidationHandler" and not an invalidation handler, the assertion is still valid after you suspend IDB and may never actually get invalidated.
> 
> Yes, should check m_shouldSuspendIDBServer
> 
> >> Source/WebKit/UIProcess/ProcessAssertion.h:75
> >> +    void setPrepareForInvalidationHandler(Function<void()>&& handler) { m_prepareForInvalidationHandler = WTFMove(handler); }
> > 
> > Can't you use setInvalidationHandler() like the others? I know very little about the prepareForInvalidationCallback so it is hard for me to understand why we need this one instead of the invalidation handler that the rest of our code relies on.
> > May be worth discussing with RunningBoard developers to see which one is best and why.
> 
> I asked RunningBoard people: 
> - (void)assertion:(RBSAssertion *)assertion didInvalidateWithError:(nullable
> NSError *)error can be called the same time as suspension happens, which
> means process can get suspended before the function returns.

[Sihui Liu]

Created attachment 433610 [details]
Patch

[Sihui Liu]

Created attachment 433613 [details]
Patch

[Chris Dumez]

Comment on attachment 433613 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2219
> +        for (auto& server : m_webIDBServers.values())

Why is this loop still here?

[Chris Dumez]

Comment on attachment 433613 [details]
Patch

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

> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:117
> +        callOnMainRunLoop([this, weakThis = WTFMove(weakThis)]() {

Seems we end up dispatching on the main thread anyway.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:274
> +    _prepareForInvalidationCallback();

Should we dispatch to the main thread here like we do in didInvalidateWithError? I'd feel better about the thread safety of this.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:395
> +    if (m_prepareForInvalidationHandler)

because I don't know how this is thread safe if this function doesn't get called on the main thread.

[Sihui Liu]

Comment on attachment 433613 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2219
>> +        for (auto& server : m_webIDBServers.values())
> 
> Why is this loop still here?

Oh!

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:274
>> +    _prepareForInvalidationCallback();
> 
> Should we dispatch to the main thread here like we do in didInvalidateWithError? I'd feel better about the thread safety of this.

Sure.

[Sihui Liu]

Created attachment 433623 [details]
Patch

[Chris Dumez]

Comment on attachment 433623 [details]
Patch

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

r=me with minor comments.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:124
>  

I think it would be helpful for debugging purposes to have release logging both in there and in resume().

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.h:56
> +    bool trySuspend(SuspendType);

I am still not super happy about the naming. Here is a proposal that I think I like better:
enum class OnlyIfIdle : bool { No, Yes };
bool suspend(OnlyIfIdle = OnlyIfIdle::No);

or
enum class SuspensionCondition : bool { Always, IfIdle };
bool suspend(SuspensionCondition = SuspendCondition::Always);

I changed my mind about the "trySuspend" naming. I think it will be clear if the enum parameter is nicely named.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:397
> +    RELEASE_LOG(ProcessSuspension, "%p - ProcessAssertion::processAssertionWillBeInvalidated() PID=%d", this, m_pid);

ASSERT(RunLoop::isMain());

[Sihui Liu]

Created attachment 433624 [details]
Patch

[EWS]

Committed r279966 (239709@main): <https://commits.webkit.org/239709@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433624 [details].