[Pepper] Use platform path for QuotaReservation::GetOpenFileHandle

[Pepper] Use platform path for QuotaReservation::GetOpenFileHandle

 * Pass platform file path instead of virtual path to fileapis::QuotaReservation.
 * This should fix the quota leak regression.

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

[tzik, 2014-01-09 07:38:33]

Hi, Bill. PTL.

We've been using virtual path to check file size growth, which is invalid.

Does the switch to reservation based quota management happen before last branch
cut?
If so, I'll file a bug and post a merge request for this.

[bbudge, 2014-01-09 13:23:50]

LGTM

The switch to using QuotaReservation happened before M33.

Since then, we have switched Pepper FileIO to Write on the plugin side. I don't
think that will affect merging this, but it's hard to be sure without trying it.

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

https://codereview.chromium.org/131043002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/quota_reservation_unittest.cc:85:
base::FilePath MakeFilePath(const base::FilePath::StringType& file_name) {
Thanks for fixing it, but I think it can be removed, as it's not called anymore.

[tzik, 2014-01-10 05:34:07]

Updated!

I made another leak fix. Could you take another look?

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

https://codereview.chromium.org/131043002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/pepper/quota_reservation_unittest.cc:85:
base::FilePath MakeFilePath(const base::FilePath::StringType& file_name) {
On 2014/01/09 13:23:50, bbudge wrote:
> Thanks for fixing it, but I think it can be removed, as it's not called
anymore.

That is used in SetFileSize(), and now used in MakeFileSystemURL.

[bbudge, 2014-01-10 06:03:13]

Still LGTM

https://codereview.chromium.org/131043002/diff/130001/content/browser/rendere...
File content/browser/renderer_host/pepper/quota_reservation.cc (right):

https://codereview.chromium.org/131043002/diff/130001/content/browser/rendere...
content/browser/renderer_host/pepper/quota_reservation.cc:86: delete it->second;
Whoa, nice catch.

[commit-bot: I haz the power, 2014-01-10 06:05:31]

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

[commit-bot: I haz the power, 2014-01-10 07:07:48]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[tzik, 2014-01-10 08:10:27]

On 2014/01/10 07:07:48, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) net_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Looking into the test failure, the root cause is the breakage of the assumption:
 actual_file_size <= maximum(max_written_offsets in FileIOs).
This is not held on append mode writing...

[tzik, 2014-01-10 12:43:59]

On 2014/01/10 08:10:27, tzik wrote:
> On 2014/01/10 07:07:48, I haz the power (commit-bot) wrote:
> > Retried try job too often on win_rel for step(s) net_unittests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
> 
> Looking into the test failure, the root cause is the breakage of the
assumption:
>  actual_file_size <= maximum(max_written_offsets in FileIOs).
> This is not held on append mode writing...

I think the proper fix of the test failure should be:
 - keep |append_amount| in addition to |max_written_offset|, and 
 - calculate upper limit of the actual file size to maximum of
|max_written_offset|s + sum of |append_amount|.

However, I'd give up to fix the test failure properly in this CL.
Even if I do, that will not be able to be merged to M33.

Let me suppress the failure for now.

[commit-bot: I haz the power, 2014-01-11 04:52:02]

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

[commit-bot: I haz the power, 2014-01-11 23:29:35]

Change committed as 244393