Bug 177972 – [WK2][iOS] Audio may keep playing for 10 seconds after closing a tab

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

More aggressively terminate child processes when the connection to their parent process is severed. Previously, we would dispatch to the main thread and then exit the process.
This would sometimes cause the process to say alive for 10 seconds until our watchdog would forcefully terminate the process. This could happen in particular when the main thread is blocked on a synchronous IPC.

[Chris Dumez]

<rdar://problem/33317607>

[Chris Dumez]

Created attachment 322923 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 322923 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        process. This could happen in particular when the main thread is blocked on a synchronous IPC.

Is it possible to fix this in sync IPC case?

Exiting the process from a background thread increases the likelihood of permanently corrupting data that was being written out at the moment. I don't think that we have a good story for that either way, but making it more likely to happen seems like the wrong direction to go in.

[Chris Dumez]

Comment on attachment 322923 [details]
Patch

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

>> Source/WebKit/ChangeLog:12
>> +        process. This could happen in particular when the main thread is blocked on a synchronous IPC.
> 
> Is it possible to fix this in sync IPC case?
> 
> Exiting the process from a background thread increases the likelihood of permanently corrupting data that was being written out at the moment. I don't think that we have a good story for that either way, but making it more likely to happen seems like the wrong direction to go in.

> Is it possible to fix this in sync IPC case?
I do not understand the question, the cases I experienced were cookie-related IPC messages (because the corresponding DOM API is synchronous). Getting rid of those synchronous IPCs is not trivial.

When I discussed this with Sam, he indicated that the WebProcess should be robust to being terminated at any point and that we used to terminate it more aggressively. Maybe I should restrict the more aggressive killing to the WebProcess and not all child processes?

[Alexey Proskuryakov]

I was thinking of sync IPC towards UI process, which would be possible to interrupt if they were blocking (it's quite likely that they are already interrupted as necessary). Cookies are of course Networking process.

> WebProcess should be robust to being terminated at any point

I should, I don't know if it is, or even if there is any way to make it so.

> we used to terminate it more aggressively

Can you find out why we no longer do?

[Alexey Proskuryakov]

*It* should

[Geoffrey Garen]

> > we used to terminate it more aggressively
> 
> Can you find out why we no longer do?

I wasn't able to find a full record of terminating more aggressively or not, but I did find a motivation for not doing so: <https://trac.webkit.org/changeset/57549/webkit>.

The concern was that exit-style termination publishes "LEAK: " reports due to WebCore objects not being deallocated before exit.

Even so, I think the test case in Radar and the FIXMEs deleted in this patch demonstrate the need to forcefully exit child processes without waiting for main thread execution. So, I think we should resurrect this patch.

[Geoffrey Garen]

Comment on attachment 322923 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 322923 [details]
Patch

Rejecting attachment 322923 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 322923, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
-- saving rejects to file Source/WebKit/StorageProcess/StorageProcess.h.rej
patching file Source/WebKit/WebProcess/WebProcess.cpp
Hunk #1 FAILED at 662.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebKit/WebProcess/WebProcess.cpp.rej
patching file Source/WebKit/WebProcess/WebProcess.h
Hunk #1 succeeded at 365 (offset 24 lines).

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Geoffrey Garen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/7555571

[Chris Dumez]

Created attachment 339484 [details]
Patch

[Chris Dumez]

Created attachment 339506 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 339506 [details]
Patch

Clearing flags on attachment: 339506

Committed r231348: <https://trac.webkit.org/changeset/231348>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.