Bug 239432

Created attachment 457769 [details]
Patch

Created attachment 457787 [details]
Patch

Created attachment 457793 [details]
Patch

Created attachment 457833 [details]
Patch

Created attachment 457840 [details]
Patch

Created attachment 457846 [details]
Patch

Created attachment 457906 [details]
Patch

Comment on attachment 457906 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        However, this is not a very reliable singal, because UI process may not send the message before process 

typo: singal

> Source/WebKit/ChangeLog:13
> +        background notification as signal of suspension. It's possible that application process holds special assertion

I can think of 2 issues here:
1. We are not supposed to use RunningBoard in child processes for security reasons AFAIK.
2. What if an app gets backgrounded, the timer starts (you suspend the db thread), then the app starts playing audible media and the RunningBoard timer stops?

Presumably then, your db thread says suspended and we'll end up with the same old hangs when the page's JS does DB operations? I say "same old" because this is the kind of bug I caused the last time I tried to rely on app visibility to make suspension decisions (as mentioned off-line).

Created attachment 458084 [details]
Patch

Created attachment 458089 [details]
Patch

Created attachment 458175 [details]
Patch

Created attachment 458176 [details]
Patch

Created attachment 458177 [details]
Patch

Created attachment 458181 [details]
Patch

<rdar://problem/92228190>

Created attachment 458297 [details]
Patch

Created attachment 458302 [details]
Patch

Comment on attachment 458302 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        However, this is not a very reliable singal, because UI process may not send the message before process 

typo: singal

> Source/WebKit/ChangeLog:20
> +        may send the suspend message earlier if ProcessThrottler says there is no activity left.

So, the odds are that the WebContent process will keep running for those 5 seconds where the network process db thread is suspended. As a result, wouldn't it cause a WebContent process hang in case of Webstorage usage (which does sync IPC to the network process).
If so, the WebProcess likely won't process the PrepareToSuspend IPC from the UIProcess. This means the WebProcess will not free its memory or make itself eligible for the freezer, making it way more likely to get jetsammed in the background.

I wouldn't want to trade network process kills on suspensions with increased WebProcess jetsams (which are more noticeable to the user IMO).

Or am I wrong and your patch deals with this in some way?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1536
> +        WTFLogAlways("sihuil: [%p]NetworkProcess::deleteWebsiteData will removeAllPushSubscriptions session[%llu]", this, sessionID.toUInt64());

Oops.

Comment on attachment 458302 [details]
Patch

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

>> Source/WebKit/ChangeLog:10
>> +        However, this is not a very reliable singal, because UI process may not send the message before process 
> 
> typo: singal

Will change.

>> Source/WebKit/ChangeLog:20
>> +        may send the suspend message earlier if ProcessThrottler says there is no activity left.
> 
> So, the odds are that the WebContent process will keep running for those 5 seconds where the network process db thread is suspended. As a result, wouldn't it cause a WebContent process hang in case of Webstorage usage (which does sync IPC to the network process).
> If so, the WebProcess likely won't process the PrepareToSuspend IPC from the UIProcess. This means the WebProcess will not free its memory or make itself eligible for the freezer, making it way more likely to get jetsammed in the background.
> 
> I wouldn't want to trade network process kills on suspensions with increased WebProcess jetsams (which are more noticeable to the user IMO).
> 
> Or am I wrong and your patch deals with this in some way?

Yes, it's possible that this increases jetsam rate of web process, when:
1. web process needs to import StorageAreaMap syncly for executing localStorage or sessionStorage operations immediately (note we import it asyncly when an origin is loaded).
2. web process keeps running in background for over 25 seconds (with some background activities) and receives assertionWillInvalidate callback (sends prepareToSuspend) after ProcessStateMonitor's timer fires (note when process gets the assertion in less than 5s to the timeout, it will not receive invalidate message, thus not sending prepareToSuspend; leading to the process be jetsammed anyways).
3. web process will not be jetsammed if prepareToSuspend is called; and will be jetsammed if prepareToSuspend is not called.
Considering the probability of meeting all conditions, the chance of increasing jetsam rate is not very likely to be noticeable.
Also, if a web process uses much memory and runs for a long time in background, I think it's not surprising that it will get jetsammed at some point.

A complete fix would be using ProcessStateMonitor to send prepareToSuspend and processDidResume messages (merging prepareToSuspend with setStorageOperationsSuspended message), and maybe ensure UI process gets the reply of prepareToSuspend from web processes before sending it to network process; but this change might be even more risky. I'd like to view the patch as a step towards the complete fix; and let's see how ProcessStateMonitor works in production.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1536
>> +        WTFLogAlways("sihuil: [%p]NetworkProcess::deleteWebsiteData will removeAllPushSubscriptions session[%llu]", this, sessionID.toUInt64());
> 
> Oops.

:(

Created attachment 458483 [details]
Patch

Created attachment 458596 [details]
Patch

Created attachment 458597 [details]
Patch

Created attachment 458599 [details]
Patch

*** This bug has been marked as a duplicate of bug 240359 ***