Bug 227552 - Take a process assertion in the network process when holding locked files
Summary: Take a process assertion in the network process when holding locked files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-30 15:49 PDT by Chris Dumez
Modified: 2021-07-02 16:06 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.49 KB, patch)
2021-06-30 16:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.26 KB, patch)
2021-06-30 16:56 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.38 KB, patch)
2021-07-01 08:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (21.38 KB, patch)
2021-07-01 10:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-06-30 15:49:12 PDT
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).
Comment 1 Chris Dumez 2021-06-30 16:01:35 PDT
Created attachment 432637 [details]
Patch
Comment 2 Chris Dumez 2021-06-30 16:56:32 PDT
Created attachment 432645 [details]
Patch
Comment 3 Chris Dumez 2021-07-01 08:49:21 PDT
Created attachment 432700 [details]
Patch
Comment 4 Chris Dumez 2021-07-01 10:13:47 PDT
Created attachment 432705 [details]
Patch
Comment 5 Sihui Liu 2021-07-02 08:27:19 PDT
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?)
Comment 6 Chris Dumez 2021-07-02 08:36:07 PDT
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.
Comment 7 Chris Dumez 2021-07-02 08:51:19 PDT
Seems we may want to revert Bug 215239 afterwards?
Comment 8 Sihui Liu 2021-07-02 10:09:43 PDT
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?
Comment 9 Sihui Liu 2021-07-02 10:20:57 PDT
(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).
Comment 10 Chris Dumez 2021-07-02 10:22:55 PDT
(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.
Comment 11 Chris Dumez 2021-07-02 10:24:17 PDT
(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.
Comment 12 Sihui Liu 2021-07-02 10:53:49 PDT
(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)?
Comment 13 Chris Dumez 2021-07-02 10:58:30 PDT
(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.
Comment 14 Sihui Liu 2021-07-02 11:07:41 PDT
(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?
Comment 15 Chris Dumez 2021-07-02 11:30:30 PDT
(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.
Comment 16 Chris Dumez 2021-07-02 11:36:55 PDT
(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.
Comment 17 Chris Dumez 2021-07-02 12:15:38 PDT
> 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?
Comment 18 EWS 2021-07-02 13:35:12 PDT
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].
Comment 19 Radar WebKit Bug Importer 2021-07-02 13:36:19 PDT
<rdar://problem/80100475>
Comment 20 Sihui Liu 2021-07-02 14:13:19 PDT
(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.
Comment 21 Chris Dumez 2021-07-02 14:31:08 PDT
(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.
Comment 22 Chris Dumez 2021-07-02 16:06:20 PDT
Follow-up Catalyst build fix in <https://commits.webkit.org/r279523>.