[Pepper] Handle plugin crash on QuotaReservation

[Pepper] Handle plugin crash on QuotaReservation

BUG=194304
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243556

[tzik, 2014-01-07 09:09:53]

Hi, Bill. Please take a look.
QuotaReservation needs another wiring to handle crash case.

I've not yet written a test for this.
Do you have any recommendation where to put the crash case test?

[bbudge, 2014-01-07 14:48:18]

Just comments. LGTM

To test this we may have to write a unit test (see the gamepad test in
content/browser/renderer_host/pepper.)

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:66:
PepperFileSystemBrowserHost::~PepperFileSystemBrowserHost() {
We have a way to detect plugin crashes in the renderer, and I thought
we would have to add that capability in the browser process too. Your
approach seems good to me though.

We probably need a comment here about the plugin crash. How about:
// If |files_| is not empty, the plugin failed to close some files. It must have
crashed.

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/quota_reservation.h (right):

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/quota_reservation.h:60: // the reserved
quota is no longer tracable.
Comment suggestion:
// Notifies underlying QuotaReservation that the associated client crashed, and
// that the reserved quota is no longer traceable.

[tzik, 2014-01-08 11:59:28]

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:66:
PepperFileSystemBrowserHost::~PepperFileSystemBrowserHost() {
On 2014/01/07 14:48:18, bbudge1 wrote:
> We have a way to detect plugin crashes in the renderer, and I thought
> we would have to add that capability in the browser process too. Your
> approach seems good to me though.
> 
> We probably need a comment here about the plugin crash. How about:
> // If |files_| is not empty, the plugin failed to close some files. It must
have
> crashed.

Done.

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/pepper/quota_reservation.h (right):

https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/quota_reservation.h:60: // the reserved
quota is no longer tracable.
On 2014/01/07 14:48:18, bbudge1 wrote:
> Comment suggestion:
> // Notifies underlying QuotaReservation that the associated client crashed,
and
> // that the reserved quota is no longer traceable.

Done.

[tzik, 2014-01-08 12:02:24]

On 2014/01/07 14:48:18, bbudge1 wrote:
> Just comments. LGTM
> 
> To test this we may have to write a unit test (see the gamepad test in
> content/browser/renderer_host/pepper.)
> 
>
https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
> (right):
> 
>
https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:66:
> PepperFileSystemBrowserHost::~PepperFileSystemBrowserHost() {
> We have a way to detect plugin crashes in the renderer, and I thought
> we would have to add that capability in the browser process too. Your
> approach seems good to me though.
> 
> We probably need a comment here about the plugin crash. How about:
> // If |files_| is not empty, the plugin failed to close some files. It must
have
> crashed.
> 
>
https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/pepper/quota_reservation.h (right):
> 
>
https://codereview.chromium.org/100963005/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/pepper/quota_reservation.h:60: // the reserved
> quota is no longer tracable.
> Comment suggestion:
> // Notifies underlying QuotaReservation that the associated client crashed,
and
> // that the reserved quota is no longer traceable.

Let me add the test in a separate CL.

[commit-bot: I haz the power, 2014-01-08 12:03:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/100963005/120001

[commit-bot: I haz the power, 2014-01-08 14:39:47]

Change committed as 243556