Bug 215239

Summary: Always suspend IDB work when network process is prepared to suspend
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: WebCore Misc.Assignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=227552
Bug Depends on:    
Bug Blocks: 197050    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Sihui Liu 2020-08-06 14:13:52 PDT
...
Comment 1 Sihui Liu 2020-08-06 14:14:28 PDT
<rdar://problem/65690450>
Comment 2 Sihui Liu 2020-08-06 14:30:18 PDT
Created attachment 406119 [details]
Patch
Comment 3 Geoffrey Garen 2020-08-07 12:56:14 PDT
Comment on attachment 406119 [details]
Patch

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

r=me

It's a little unfortunate that all transactions started before suspension will be aborted, in contrast to transactions started after suspension, which will just be paused and delayed. Is there a way to delay in progress transactions instead of aborting them? I guess not, since an in progress transaction holds a database file lock? Would it be crazy to abort a transaction without signaling an error and then put it back in the queue to try again when we resume?

> Source/WebKit/ChangeLog:9
> +        We do not suspend IDB work in network process when there is ongoing transaction because network process is 

network process => the network process
ongoing transaction => an ongoing transaction
network process => the network process

> Source/WebKit/ChangeLog:10
> +        going to ask UI process to hold process assertion for it. However, it is possible that the request from network

UI process => the UI process
process assertion => a process assertion
network => the network

> Source/WebKit/ChangeLog:11
> +        process does not reach UI process before UI process thinks think it is Okay to put network process to suspend. 

UI process => the UI process
UI process => the UI process
thinks think -> thinks
Okay => okay
put network process to suspend => suspend the network process

I believe it's runningboard, and not the UI process, that ultimately suspends the network process. So, I think this comment should talk about runningboard. Or maybe you could talk about the UI process dropping its foreground assertion on the network process? Is that what ultimately triggers runningboard to suspend the network process?

> Source/WebKit/ChangeLog:12
> +        In this case, IDB in network process can continue its transaction, holding file locks, and network process will 

network process => the network process
network process => the network process

> Source/WebKit/ChangeLog:13
> +        be killed when it becomes suspened. Network process crash can cause worse result than aborting ongoing 

Network process crash => A network process crash
worse result => a worse result

> Source/WebKit/ChangeLog:15
> +        let's just suspend IDB work when network process receives prepareToSuspend.

network process => the network process
Comment 4 Geoffrey Garen 2020-08-07 12:57:04 PDT
A related reason why aborting does not feel great: It will trigger more code to run when we are supposed to be suspending, since it will fire an event in the WebContent process.
Comment 5 Sihui Liu 2020-08-10 09:16:10 PDT
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 406119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406119&action=review
> 
> r=me
> 
> It's a little unfortunate that all transactions started before suspension
> will be aborted, in contrast to transactions started after suspension, which
> will just be paused and delayed. Is there a way to delay in progress
> transactions instead of aborting them? I guess not, since an in progress
> transaction holds a database file lock? Would it be crazy to abort a
> transaction without signaling an error and then put it back in the queue to
> try again when we resume?
> 
It can be complicated. We would need to record the requests/operations of started transactions, together with their result, so we can replay them after process is resumed. But what if result of the second execution is different from the first execution; e.g. we may fire success event on some request(operation), event handler is called and a new request is created in the event handler.

> > Source/WebKit/ChangeLog:9
> > +        We do not suspend IDB work in network process when there is ongoing transaction because network process is 
> 
> network process => the network process
> ongoing transaction => an ongoing transaction
> network process => the network process
> 

Okay.

> > Source/WebKit/ChangeLog:10
> > +        going to ask UI process to hold process assertion for it. However, it is possible that the request from network
> 
> UI process => the UI process
> process assertion => a process assertion
> network => the network
> 
Okay.

> > Source/WebKit/ChangeLog:11
> > +        process does not reach UI process before UI process thinks think it is Okay to put network process to suspend. 
> 
> UI process => the UI process
> UI process => the UI process
> thinks think -> thinks
> Okay => okay
> put network process to suspend => suspend the network process
> 
> I believe it's runningboard, and not the UI process, that ultimately
> suspends the network process. So, I think this comment should talk about
> runningboard. Or maybe you could talk about the UI process dropping its
> foreground assertion on the network process? Is that what ultimately
> triggers runningboard to suspend the network process?
> 

You are right, it's RunningBoard that decides it's Okay to suspend the network process after UI process drops process assertion for network process. Will update.

> > Source/WebKit/ChangeLog:12
> > +        In this case, IDB in network process can continue its transaction, holding file locks, and network process will 
> 
> network process => the network process
> network process => the network process
> 
Okay.

> > Source/WebKit/ChangeLog:13
> > +        be killed when it becomes suspened. Network process crash can cause worse result than aborting ongoing 
> 
> Network process crash => A network process crash
> worse result => a worse result
> 
Okay.

> > Source/WebKit/ChangeLog:15
> > +        let's just suspend IDB work when network process receives prepareToSuspend.
> 
> network process => the network process
Okay.
Comment 6 Sihui Liu 2020-08-10 09:24:59 PDT
(In reply to Geoffrey Garen from comment #4)
> A related reason why aborting does not feel great: It will trigger more code
> to run when we are supposed to be suspending, since it will fire an event in
> the WebContent process.

I agree. Another possible solution would be to ensure the network process sends request for holding background assertion before the network process replies to the prepareToSuspend message. Current patch is just less risky and more straightforward to fix the crashes.
Comment 7 Sihui Liu 2020-08-10 09:28:22 PDT
Created attachment 406299 [details]
Patch for landing
Comment 8 Geoffrey Garen 2020-08-10 09:33:22 PDT
I tend to agree that this approach is better (more likely to avoid a crash) than relying on the network process acquiring a background assertion.
Comment 9 EWS 2020-08-10 09:51:23 PDT
Committed r265433: <https://trac.webkit.org/changeset/265433>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406299 [details].
Comment 10 Chris Dumez 2020-08-10 09:54:44 PDT
Given that we had a bug that caused the PrepareToSuspend IPC to not get sent to the network process until a few days ago, I am not sure we needed to make this change.

I believe the fix to actually send the PrepareToSuspend IPC has not been submitted yet so making changes to our behavior when the PrepareToSuspend IPC gets received to probably not super helpful.
Comment 11 Chris Dumez 2020-08-10 10:10:29 PDT
(In reply to Chris Dumez from comment #10)
> Given that we had a bug that caused the PrepareToSuspend IPC to not get sent
> to the network process until a few days ago, I am not sure we needed to make
> this change.
> 
> I believe the fix to actually send the PrepareToSuspend IPC has not been
> submitted yet so making changes to our behavior when the PrepareToSuspend
> IPC gets received to probably not super helpful.

http://trac.webkit.org/r265162 fixed the bug where the PrepareToSuspend IPC not getting sent in most cases.
Comment 12 Sihui Liu 2020-08-10 10:18:09 PDT
(In reply to Chris Dumez from comment #11)
> (In reply to Chris Dumez from comment #10)
> > Given that we had a bug that caused the PrepareToSuspend IPC to not get sent
> > to the network process until a few days ago, I am not sure we needed to make
> > this change.
> > 
> > I believe the fix to actually send the PrepareToSuspend IPC has not been
> > submitted yet so making changes to our behavior when the PrepareToSuspend
> > IPC gets received to probably not super helpful.
> 
> http://trac.webkit.org/r265162 fixed the bug where the PrepareToSuspend IPC
> not getting sent in most cases.

I tested on ToT WebKit last Friday and reproduced the crash with this log:
com.apple.WebKit.Networking (WebCore): SQLiteTransaction::begin()
com.apple.WebKit.Networking (WebKit): NetworkProcess::prepareToSuspend(), isSuspensionImminent: 0
com.apple.WebKit.Networking (WebKit): WebSQLiteDatabaseTracker::setIsSuspended isSuspended[1]
com.apple.WebKit.Networking (WebKit): NetworkProcess::prepareToSuspend() Process is ready to suspend