Take a process assertion in the network process when holding locked files

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Take a process assertion in the network process when holding locked files. Previously, we'd send an IPC to the UIProcess and the UIProcess would take the assertion on behalf of the network process. However, our previous approach was racy and could cause the network process to get suspended while still holding locked files (which would get it killed).

[Chris Dumez]

Created attachment 432637 [details]
Patch

[Chris Dumez]

Created attachment 432645 [details]
Patch

[Chris Dumez]

Created attachment 432700 [details]
Patch

[Chris Dumez]

Created attachment 432705 [details]
Patch

[Sihui Liu]

Comment on attachment 432705 [details]
Patch

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

> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);

Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)

[Chris Dumez]

Comment on attachment 432705 [details]
Patch

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

>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
> 
> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)

AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).

> Does UI process always send prepareToSuspend before marking network process suspendible?
Yes.

[Chris Dumez]

Seems we may want to revert Bug 215239 afterwards?

[Sihui Liu]

Comment on attachment 432705 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
>>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
>> 
>> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
>> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)
> 
> AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
> We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).

My understanding is prepareToSuspend means the process should be safe to suspend afterwards, so we make sure no SQLite activities after this by suspending storage threads (no new activity can start) and aborting ongoing transactions (no old activity can continue). 

With current patch, I think our behavior is: network process's database activities gets aborted whenever UI process thinks it can be suspended (prepareToSuspend). Since UI process does not know about the database state in network process now, it decides the suspended state by other network process's activities that requires assertion. Then why do we acquire assertion for all database activities in network process? If we want make sure it has enough time to abort, shouldn't we only acquire assertion in NetworkProcess::prepareToSuspend and release it in ~CallbackAggregator?

[Sihui Liu]

(In reply to Chris Dumez from comment #7)
> Seems we may want to revert Bug 215239 afterwards?

Why? It seems imminent or not, UI process can drop the assertion of network process. Then assertion in network process can expire/time out after that, and there's no guarantee IDB will finish in timeout (and we don't have invalidation handler here).

[Chris Dumez]

(In reply to Sihui Liu from comment #8)
> Comment on attachment 432705 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432705&action=review
> 
> >>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
> >>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
> >> 
> >> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
> >> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)
> > 
> > AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
> > We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).
> 
> My understanding is prepareToSuspend means the process should be safe to
> suspend afterwards, so we make sure no SQLite activities after this by
> suspending storage threads (no new activity can start) and aborting ongoing
> transactions (no old activity can continue). 
> 
> With current patch, I think our behavior is: network process's database
> activities gets aborted whenever UI process thinks it can be suspended
> (prepareToSuspend). Since UI process does not know about the database state
> in network process now, it decides the suspended state by other network
> process's activities that requires assertion. Then why do we acquire
> assertion for all database activities in network process? If we want make
> sure it has enough time to abort, shouldn't we only acquire assertion in
> NetworkProcess::prepareToSuspend and release it in ~CallbackAggregator?

With your alternative proposal, there is no guarantee the network process will receive the PrepareToSuspend before suspension, especially in the imminent suspension case. In the imminent suspension case, the UIProcess will get suspended almost immediately. It does its best to send out an imminent suspension IPC to its child processes before getting suspended but at this point RunningBoard is not giving us a choice and the UIProcess might get suspended before it has a chance to send out the PrepareToSuspend IPC. There is also a chance the child process gets suspended before it has a chance to process the PrepareToSuspend IPC.

Note that this is exactly the issue we see in the logs and this is exactly WHY I am proposing this change.

[Chris Dumez]

(In reply to Sihui Liu from comment #9)
> (In reply to Chris Dumez from comment #7)
> > Seems we may want to revert Bug 215239 afterwards?
> 
> Why? It seems imminent or not, UI process can drop the assertion of network
> process. Then assertion in network process can expire/time out after that,
> and there's no guarantee IDB will finish in timeout (and we don't have
> invalidation handler here).

I meant reverting Bug 215239 and adding an invalidation handler for the new database activity assertion to abort transactions.

[Sihui Liu]

(In reply to Chris Dumez from comment #10)
> (In reply to Sihui Liu from comment #8)
> > Comment on attachment 432705 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=432705&action=review
> > 
> > >>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
> > >>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
> > >> 
> > >> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
> > >> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)
> > > 
> > > AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
> > > We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).
> > 
> > My understanding is prepareToSuspend means the process should be safe to
> > suspend afterwards, so we make sure no SQLite activities after this by
> > suspending storage threads (no new activity can start) and aborting ongoing
> > transactions (no old activity can continue). 
> > 
> > With current patch, I think our behavior is: network process's database
> > activities gets aborted whenever UI process thinks it can be suspended
> > (prepareToSuspend). Since UI process does not know about the database state
> > in network process now, it decides the suspended state by other network
> > process's activities that requires assertion. Then why do we acquire
> > assertion for all database activities in network process? If we want make
> > sure it has enough time to abort, shouldn't we only acquire assertion in
> > NetworkProcess::prepareToSuspend and release it in ~CallbackAggregator?
> 
> With your alternative proposal, there is no guarantee the network process
> will receive the PrepareToSuspend before suspension, especially in the
> imminent suspension case. In the imminent suspension case, the UIProcess
> will get suspended almost immediately. It does its best to send out an
> imminent suspension IPC to its child processes before getting suspended but
> at this point RunningBoard is not giving us a choice and the UIProcess might
> get suspended before it has a chance to send out the PrepareToSuspend IPC.
> There is also a chance the child process gets suspended before it has a
> chance to process the PrepareToSuspend IPC.
> 
> Note that this is exactly the issue we see in the logs and this is exactly
> WHY I am proposing this change.

I see, so the purpose of assertion in network process is to make sure process is not suspended before receiving prepareToSuspend message. That makes sense. 

Have you manually tested the change? 

I notice a thing that may make this patch not work as expected. If network process acquires the assertion on first transaction, get a prepareToSuspend, get a processDidResume, start a new transaction and new assertion may not be acquired, if the first transaction is not aborted or committed (e.g. LocalStorage commits transaction after some duration)?

[Chris Dumez]

(In reply to Sihui Liu from comment #12)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Sihui Liu from comment #8)
> > > Comment on attachment 432705 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=432705&action=review
> > > 
> > > >>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
> > > >>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
> > > >> 
> > > >> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
> > > >> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)
> > > > 
> > > > AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
> > > > We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).
> > > 
> > > My understanding is prepareToSuspend means the process should be safe to
> > > suspend afterwards, so we make sure no SQLite activities after this by
> > > suspending storage threads (no new activity can start) and aborting ongoing
> > > transactions (no old activity can continue). 
> > > 
> > > With current patch, I think our behavior is: network process's database
> > > activities gets aborted whenever UI process thinks it can be suspended
> > > (prepareToSuspend). Since UI process does not know about the database state
> > > in network process now, it decides the suspended state by other network
> > > process's activities that requires assertion. Then why do we acquire
> > > assertion for all database activities in network process? If we want make
> > > sure it has enough time to abort, shouldn't we only acquire assertion in
> > > NetworkProcess::prepareToSuspend and release it in ~CallbackAggregator?
> > 
> > With your alternative proposal, there is no guarantee the network process
> > will receive the PrepareToSuspend before suspension, especially in the
> > imminent suspension case. In the imminent suspension case, the UIProcess
> > will get suspended almost immediately. It does its best to send out an
> > imminent suspension IPC to its child processes before getting suspended but
> > at this point RunningBoard is not giving us a choice and the UIProcess might
> > get suspended before it has a chance to send out the PrepareToSuspend IPC.
> > There is also a chance the child process gets suspended before it has a
> > chance to process the PrepareToSuspend IPC.
> > 
> > Note that this is exactly the issue we see in the logs and this is exactly
> > WHY I am proposing this change.
> 
> I see, so the purpose of assertion in network process is to make sure
> process is not suspended before receiving prepareToSuspend message. That
> makes sense. 
> 
> Have you manually tested the change? 

Of course.

> 
> I notice a thing that may make this patch not work as expected. If network
> process acquires the assertion on first transaction, get a prepareToSuspend,
> get a processDidResume, start a new transaction and new assertion may not be
> acquired, if the first transaction is not aborted or committed (e.g.
> LocalStorage commits transaction after some duration)?

Could you elaborate on this? I don't think I understand. In my patch, we always take an assertion when a transaction starts (unless we already hold a valid assertion), as far as I can tell.

[Sihui Liu]

(In reply to Chris Dumez from comment #13)
> (In reply to Sihui Liu from comment #12)
> > (In reply to Chris Dumez from comment #10)
> > > (In reply to Sihui Liu from comment #8)
> > > > Comment on attachment 432705 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=432705&action=review
> > > > 
> > > > >>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
> > > > >>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
> > > > >> 
> > > > >> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
> > > > >> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)
> > > > > 
> > > > > AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
> > > > > We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).
> > > > 
> > > > My understanding is prepareToSuspend means the process should be safe to
> > > > suspend afterwards, so we make sure no SQLite activities after this by
> > > > suspending storage threads (no new activity can start) and aborting ongoing
> > > > transactions (no old activity can continue). 
> > > > 
> > > > With current patch, I think our behavior is: network process's database
> > > > activities gets aborted whenever UI process thinks it can be suspended
> > > > (prepareToSuspend). Since UI process does not know about the database state
> > > > in network process now, it decides the suspended state by other network
> > > > process's activities that requires assertion. Then why do we acquire
> > > > assertion for all database activities in network process? If we want make
> > > > sure it has enough time to abort, shouldn't we only acquire assertion in
> > > > NetworkProcess::prepareToSuspend and release it in ~CallbackAggregator?
> > > 
> > > With your alternative proposal, there is no guarantee the network process
> > > will receive the PrepareToSuspend before suspension, especially in the
> > > imminent suspension case. In the imminent suspension case, the UIProcess
> > > will get suspended almost immediately. It does its best to send out an
> > > imminent suspension IPC to its child processes before getting suspended but
> > > at this point RunningBoard is not giving us a choice and the UIProcess might
> > > get suspended before it has a chance to send out the PrepareToSuspend IPC.
> > > There is also a chance the child process gets suspended before it has a
> > > chance to process the PrepareToSuspend IPC.
> > > 
> > > Note that this is exactly the issue we see in the logs and this is exactly
> > > WHY I am proposing this change.
> > 
> > I see, so the purpose of assertion in network process is to make sure
> > process is not suspended before receiving prepareToSuspend message. That
> > makes sense. 
> > 
> > Have you manually tested the change? 
> 
> Of course.
> 
> > 
> > I notice a thing that may make this patch not work as expected. If network
> > process acquires the assertion on first transaction, get a prepareToSuspend,
> > get a processDidResume, start a new transaction and new assertion may not be
> > acquired, if the first transaction is not aborted or committed (e.g.
> > LocalStorage commits transaction after some duration)?
> 
> Could you elaborate on this? I don't think I understand. In my patch, we
> always take an assertion when a transaction starts (unless we already hold a
> valid assertion), as far as I can tell.

Let's say a LocalStorage transaction begin() (schedule a 500ms task to commit), transaction count is 1 and assertion is acquired.
Network process receives prepareToSuspend, hangs the storage thread,
After a long time, Network process receives processDidResume. (assertion is already timed out)
IDB thread begin() a new transaction, transaction count is 2. No assertion at this point.
Then LocalStorage transaction commit(), transaction count is 1. No assertion at this point.
So prepareToSuspend may still be not received?

[Chris Dumez]

(In reply to Sihui Liu from comment #14)
> (In reply to Chris Dumez from comment #13)
> > (In reply to Sihui Liu from comment #12)
> > > (In reply to Chris Dumez from comment #10)
> > > > (In reply to Sihui Liu from comment #8)
> > > > > Comment on attachment 432705 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=432705&action=review
> > > > > 
> > > > > >>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
> > > > > >>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
> > > > > >> 
> > > > > >> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
> > > > > >> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)
> > > > > > 
> > > > > > AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
> > > > > > We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).
> > > > > 
> > > > > My understanding is prepareToSuspend means the process should be safe to
> > > > > suspend afterwards, so we make sure no SQLite activities after this by
> > > > > suspending storage threads (no new activity can start) and aborting ongoing
> > > > > transactions (no old activity can continue). 
> > > > > 
> > > > > With current patch, I think our behavior is: network process's database
> > > > > activities gets aborted whenever UI process thinks it can be suspended
> > > > > (prepareToSuspend). Since UI process does not know about the database state
> > > > > in network process now, it decides the suspended state by other network
> > > > > process's activities that requires assertion. Then why do we acquire
> > > > > assertion for all database activities in network process? If we want make
> > > > > sure it has enough time to abort, shouldn't we only acquire assertion in
> > > > > NetworkProcess::prepareToSuspend and release it in ~CallbackAggregator?
> > > > 
> > > > With your alternative proposal, there is no guarantee the network process
> > > > will receive the PrepareToSuspend before suspension, especially in the
> > > > imminent suspension case. In the imminent suspension case, the UIProcess
> > > > will get suspended almost immediately. It does its best to send out an
> > > > imminent suspension IPC to its child processes before getting suspended but
> > > > at this point RunningBoard is not giving us a choice and the UIProcess might
> > > > get suspended before it has a chance to send out the PrepareToSuspend IPC.
> > > > There is also a chance the child process gets suspended before it has a
> > > > chance to process the PrepareToSuspend IPC.
> > > > 
> > > > Note that this is exactly the issue we see in the logs and this is exactly
> > > > WHY I am proposing this change.
> > > 
> > > I see, so the purpose of assertion in network process is to make sure
> > > process is not suspended before receiving prepareToSuspend message. That
> > > makes sense. 
> > > 
> > > Have you manually tested the change? 
> > 
> > Of course.
> > 
> > > 
> > > I notice a thing that may make this patch not work as expected. If network
> > > process acquires the assertion on first transaction, get a prepareToSuspend,
> > > get a processDidResume, start a new transaction and new assertion may not be
> > > acquired, if the first transaction is not aborted or committed (e.g.
> > > LocalStorage commits transaction after some duration)?
> > 
> > Could you elaborate on this? I don't think I understand. In my patch, we
> > always take an assertion when a transaction starts (unless we already hold a
> > valid assertion), as far as I can tell.
> 
> Let's say a LocalStorage transaction begin() (schedule a 500ms task to
> commit), transaction count is 1 and assertion is acquired.
> Network process receives prepareToSuspend, hangs the storage thread,
> After a long time, Network process receives processDidResume. (assertion is
> already timed out)
> IDB thread begin() a new transaction, transaction count is 2. No assertion
> at this point.
> Then LocalStorage transaction commit(), transaction count is 1. No assertion
> at this point.
> So prepareToSuspend may still be not received?

Oh, I understand now. Thanks for explaining. I agree this is a problem. However, this is a problem right now without my change.

PrepareToSuspend() should really cause the LocalStorageDatabase to commit its transaction *before* it suspends the storage thread. Suspending the thread (when there is a scheduled task to commit a transaction) actually prevents the transaction from getting committed and guarantees the process will suspend with a locked file.

I can look into fixing this one separately as this doesn't really have much to do with this patch.

[Chris Dumez]

(In reply to Chris Dumez from comment #15)
> (In reply to Sihui Liu from comment #14)
> > (In reply to Chris Dumez from comment #13)
> > > (In reply to Sihui Liu from comment #12)
> > > > (In reply to Chris Dumez from comment #10)
> > > > > (In reply to Sihui Liu from comment #8)
> > > > > > Comment on attachment 432705 [details]
> > > > > > Patch
> > > > > > 
> > > > > > View in context:
> > > > > > https://bugs.webkit.org/attachment.cgi?id=432705&action=review
> > > > > > 
> > > > > > >>> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:112
> > > > > > >>> +    m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
> > > > > > >> 
> > > > > > >> Do we want to set invalidation handler to abort transactions, instead of doing it in NetworkProcess::prepareToSuspend? Since prepareToSuspend may be sent more often now
> > > > > > >> Does UI process always send prepareToSuspend before marking network process suspendible? (Is it possible network process starts transaction and acquires assertion after UI process acquires suspended assertion for network process?)
> > > > > > > 
> > > > > > > AS I mentioned to you offline, I think NetworkProcess::prepareToSuspend should suspend the storage threads, not abort the transactions, at least in the non-imminent suspension case.
> > > > > > > We could indeed add a invalidation handler here to abort transactions. Maybe you can do this in a follow-up since you are more familiar with that? Similarly, I was hoping you could make the changes to prepareToSuspend (to not abort transactions so eagerly).
> > > > > > 
> > > > > > My understanding is prepareToSuspend means the process should be safe to
> > > > > > suspend afterwards, so we make sure no SQLite activities after this by
> > > > > > suspending storage threads (no new activity can start) and aborting ongoing
> > > > > > transactions (no old activity can continue). 
> > > > > > 
> > > > > > With current patch, I think our behavior is: network process's database
> > > > > > activities gets aborted whenever UI process thinks it can be suspended
> > > > > > (prepareToSuspend). Since UI process does not know about the database state
> > > > > > in network process now, it decides the suspended state by other network
> > > > > > process's activities that requires assertion. Then why do we acquire
> > > > > > assertion for all database activities in network process? If we want make
> > > > > > sure it has enough time to abort, shouldn't we only acquire assertion in
> > > > > > NetworkProcess::prepareToSuspend and release it in ~CallbackAggregator?
> > > > > 
> > > > > With your alternative proposal, there is no guarantee the network process
> > > > > will receive the PrepareToSuspend before suspension, especially in the
> > > > > imminent suspension case. In the imminent suspension case, the UIProcess
> > > > > will get suspended almost immediately. It does its best to send out an
> > > > > imminent suspension IPC to its child processes before getting suspended but
> > > > > at this point RunningBoard is not giving us a choice and the UIProcess might
> > > > > get suspended before it has a chance to send out the PrepareToSuspend IPC.
> > > > > There is also a chance the child process gets suspended before it has a
> > > > > chance to process the PrepareToSuspend IPC.
> > > > > 
> > > > > Note that this is exactly the issue we see in the logs and this is exactly
> > > > > WHY I am proposing this change.
> > > > 
> > > > I see, so the purpose of assertion in network process is to make sure
> > > > process is not suspended before receiving prepareToSuspend message. That
> > > > makes sense. 
> > > > 
> > > > Have you manually tested the change? 
> > > 
> > > Of course.
> > > 
> > > > 
> > > > I notice a thing that may make this patch not work as expected. If network
> > > > process acquires the assertion on first transaction, get a prepareToSuspend,
> > > > get a processDidResume, start a new transaction and new assertion may not be
> > > > acquired, if the first transaction is not aborted or committed (e.g.
> > > > LocalStorage commits transaction after some duration)?
> > > 
> > > Could you elaborate on this? I don't think I understand. In my patch, we
> > > always take an assertion when a transaction starts (unless we already hold a
> > > valid assertion), as far as I can tell.
> > 
> > Let's say a LocalStorage transaction begin() (schedule a 500ms task to
> > commit), transaction count is 1 and assertion is acquired.
> > Network process receives prepareToSuspend, hangs the storage thread,
> > After a long time, Network process receives processDidResume. (assertion is
> > already timed out)
> > IDB thread begin() a new transaction, transaction count is 2. No assertion
> > at this point.
> > Then LocalStorage transaction commit(), transaction count is 1. No assertion
> > at this point.
> > So prepareToSuspend may still be not received?
> 
> Oh, I understand now. Thanks for explaining. I agree this is a problem.
> However, this is a problem right now without my change.
> 
> PrepareToSuspend() should really cause the LocalStorageDatabase to commit
> its transaction *before* it suspends the storage thread. Suspending the
> thread (when there is a scheduled task to commit a transaction) actually
> prevents the transaction from getting committed and guarantees the process
> will suspend with a locked file.
> 
> I can look into fixing this one separately as this doesn't really have much
> to do with this patch.

I filed Bug 227632 and am working on a patch.

[Chris Dumez]

> I meant reverting Bug 215239 and adding an invalidation handler for the new
> database activity assertion to abort transactions.

@Sihui, do you think we should make this change in a follow-up?
If you do, go you want to do it or should I?

[EWS]

Committed r279514 (239366@main): <https://commits.webkit.org/239366@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/80100475>

[Sihui Liu]

(In reply to Chris Dumez from comment #17)
> > I meant reverting Bug 215239 and adding an invalidation handler for the new
> > database activity assertion to abort transactions.
> 
> @Sihui, do you think we should make this change in a follow-up?
> If you do, go you want to do it or should I?

Yes, follow-up to not eagerly abort transactions on prepareToSuspend. This change I am not sure; seems better if we can do the same thing for all storage threads

I will follow up.

[Chris Dumez]

(In reply to Sihui Liu from comment #20)
> (In reply to Chris Dumez from comment #17)
> > > I meant reverting Bug 215239 and adding an invalidation handler for the new
> > > database activity assertion to abort transactions.
> > 
> > @Sihui, do you think we should make this change in a follow-up?
> > If you do, go you want to do it or should I?
> 
> Yes, follow-up to not eagerly abort transactions on prepareToSuspend. This
> change I am not sure; seems better if we can do the same thing for all
> storage threads
> 
> I will follow up.

Ok, thanks.

[Chris Dumez]

Follow-up Catalyst build fix in <https://commits.webkit.org/r279523>.