[Pepper] Wire up append mode writing support of FileIO

[Pepper] Wire up append mode writing support of FileIO

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

[tzik, 2014-01-20 12:19:49]

Hi.

Bill:
This is wiring part append mode write from/to Pepper IPC, and hopefully last
part of append mode support. Please take a look.

Justin:
Please take a look to ppapi_messages.h part.
Previous code sends maximum written offset to a file in ReserveQuota IPC and
CloseFile IPC, and after this change, we'll send the amount of append-mode write
separately.

[bbudge, 2014-01-21 18:03:50]

https://codereview.chromium.org/130053003/diff/90005/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/130053003/diff/90005/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:337: int64_t
max_written_offset) {
function signature isn't updated to new one.

https://codereview.chromium.org/130053003/diff/90005/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/130053003/diff/90005/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:1330: ppapi::FileGrowthMap /* file_growth */)
s/file_growth/file_growths or file_growth_map

[jschuh, 2014-01-22 00:07:12]

Could I get some more context here, such as an implementation bug describing how
this should work?

[tzik, 2014-01-22 07:16:58]

On 2014/01/22 00:07:12, Justin Schuh wrote:
> Could I get some more context here, such as an implementation bug describing
how
> this should work?

Sure, let me update the tracking bug: http://crbug.com/334171

[tzik, 2014-01-22 07:17:10]

https://codereview.chromium.org/130053003/diff/90005/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/130053003/diff/90005/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_io_host.cc:337: int64_t
max_written_offset) {
On 2014/01/21 18:03:51, bbudge wrote:
> function signature isn't updated to new one.

...!
I compiled on old source..

Updated.

https://codereview.chromium.org/130053003/diff/90005/ppapi/proxy/ppapi_messag...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/130053003/diff/90005/ppapi/proxy/ppapi_messag...
ppapi/proxy/ppapi_messages.h:1330: ppapi::FileGrowthMap /* file_growth */)
On 2014/01/21 18:03:51, bbudge wrote:
> s/file_growth/file_growths or file_growth_map

Done.

[bbudge, 2014-01-22 20:47:27]

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/file_system...
File ppapi/proxy/file_system_resource.h (right):

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/file_system...
ppapi/proxy/file_system_resource.h:20: #include
"ppapi/shared_impl/file_growth.h"
This doesn't seem to be needed in the header.

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:399: FileGrowth(trusted_max_written_offset,
0))));
We could probably change the FileIO class to know about append mode, and change
NaClIPCAdapter to return 'bytes_written' so we can have an accurate value for
this and can create a FileGrowth that has both of these values. Would that be
helpful, or is this good enough for the quota system?

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1333: ppapi::FileSizeMap /* max_written_offsets */)
I'm wondering if we should now call this 'file_sizes', both here and in the host
and resource code.

[bbudge, 2014-01-22 21:02:29]

Another thing. I think you'll need to change NaClMessageScannerTest:

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/nacl_m...

[jschuh, 2014-01-23 05:09:39]

ipc security lgtm

[tzik, 2014-01-23 08:42:54]

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/file_system...
File ppapi/proxy/file_system_resource.h (right):

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/file_system...
ppapi/proxy/file_system_resource.h:20: #include
"ppapi/shared_impl/file_growth.h"
On 2014/01/22 20:47:27, bbudge wrote:
> This doesn't seem to be needed in the header.

Done.

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:399: FileGrowth(trusted_max_written_offset,
0))));
On 2014/01/22 20:47:27, bbudge wrote:
> We could probably change the FileIO class to know about append mode, and
change
> NaClIPCAdapter to return 'bytes_written' so we can have an accurate value for
> this and can create a FileGrowth that has both of these values. Would that be
> helpful, or is this good enough for the quota system?

Ah, I didn't know we provide quota managed writable file descriptor to NaCl.
It has a separate mechanism to RequestOSFileHandle and is only for trusted
plugins, right?

Yes, we should use QuotaReservation for them too.

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:1333: ppapi::FileSizeMap /* max_written_offsets */)
On 2014/01/22 20:47:27, bbudge wrote:
> I'm wondering if we should now call this 'file_sizes', both here and in the
host
> and resource code.

Done.

[bbudge, 2014-01-23 11:50:06]

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:399: FileGrowth(trusted_max_written_offset,
0))));
On 2014/01/23 08:42:54, tzik wrote:
> On 2014/01/22 20:47:27, bbudge wrote:
> > We could probably change the FileIO class to know about append mode, and
> change
> > NaClIPCAdapter to return 'bytes_written' so we can have an accurate value
for
> > this and can create a FileGrowth that has both of these values. Would that
be
> > helpful, or is this good enough for the quota system?
> 
> Ah, I didn't know we provide quota managed writable file descriptor to NaCl.
> It has a separate mechanism to RequestOSFileHandle and is only for trusted
> plugins, right?
> 
> Yes, we should use QuotaReservation for them too.

The NaCl app gets a file descriptor that is backed by a NaClDescQuota in the
trusted part of the plugin process. That holds the real file descriptor / handle
and does the quota verification. It knows both the actual offset and length of
the write and can control how much is written. Here's the part that checks
quota:

https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/lo...

So you'd change the FileIO class to have a write mode and a FileGrowth field,
and change the Grow method.

[tzik, 2014-01-24 05:59:34]

On 2014/01/23 11:50:06, bbudge wrote:
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> File ppapi/proxy/nacl_message_scanner.cc (right):
> 
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> ppapi/proxy/nacl_message_scanner.cc:399:
FileGrowth(trusted_max_written_offset,
> 0))));
> On 2014/01/23 08:42:54, tzik wrote:
> > On 2014/01/22 20:47:27, bbudge wrote:
> > > We could probably change the FileIO class to know about append mode, and
> > change
> > > NaClIPCAdapter to return 'bytes_written' so we can have an accurate value
> for
> > > this and can create a FileGrowth that has both of these values. Would that
> be
> > > helpful, or is this good enough for the quota system?
> > 
> > Ah, I didn't know we provide quota managed writable file descriptor to NaCl.
> > It has a separate mechanism to RequestOSFileHandle and is only for trusted
> > plugins, right?
> > 
> > Yes, we should use QuotaReservation for them too.
> 
> The NaCl app gets a file descriptor that is backed by a NaClDescQuota in the
> trusted part of the plugin process. That holds the real file descriptor /
handle
> and does the quota verification. It knows both the actual offset and length of
> the write and can control how much is written. Here's the part that checks
> quota:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/lo...
> 
> So you'd change the FileIO class to have a write mode and a FileGrowth field,
> and change the Grow method.

Looking at them, it checks/consumes quota before writing, but it doesn't request
quota when it need.
Is it working now?

[bbudge, 2014-01-24 13:12:42]

On 2014/01/24 05:59:34, tzik wrote:
> On 2014/01/23 11:50:06, bbudge wrote:
> >
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> > File ppapi/proxy/nacl_message_scanner.cc (right):
> > 
> >
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> > ppapi/proxy/nacl_message_scanner.cc:399:
> FileGrowth(trusted_max_written_offset,
> > 0))));
> > On 2014/01/23 08:42:54, tzik wrote:
> > > On 2014/01/22 20:47:27, bbudge wrote:
> > > > We could probably change the FileIO class to know about append mode, and
> > > change
> > > > NaClIPCAdapter to return 'bytes_written' so we can have an accurate
value
> > for
> > > > this and can create a FileGrowth that has both of these values. Would
that
> > be
> > > > helpful, or is this good enough for the quota system?
> > > 
> > > Ah, I didn't know we provide quota managed writable file descriptor to
NaCl.
> > > It has a separate mechanism to RequestOSFileHandle and is only for trusted
> > > plugins, right?
> > > 
> > > Yes, we should use QuotaReservation for them too.
> > 
> > The NaCl app gets a file descriptor that is backed by a NaClDescQuota in the
> > trusted part of the plugin process. That holds the real file descriptor /
> handle
> > and does the quota verification. It knows both the actual offset and length
of
> > the write and can control how much is written. Here's the part that checks
> > quota:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/lo...
> > 
> > So you'd change the FileIO class to have a write mode and a FileGrowth
field,
> > and change the Grow method.
> 
> Looking at them, it checks/consumes quota before writing, but it doesn't
request
> quota when it need.
> Is it working now?

Yes, it should be working now. This layer just audits the message traffic
between the plugin and host. It intercepts the quota reservation message reply
to get the amount. The NaClDescQuota reports how much the plugin is consuming,
which the auditing layer deducts from the reservation. This should mimic what
happens in the proxy. We have to audit because the plugin could write with its
file handle without running the proxy code.

[tzik, 2014-01-27 06:24:23]

On 2014/01/24 13:12:42, bbudge wrote:
> On 2014/01/24 05:59:34, tzik wrote:
> > On 2014/01/23 11:50:06, bbudge wrote:
> > >
> >
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> > > File ppapi/proxy/nacl_message_scanner.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> > > ppapi/proxy/nacl_message_scanner.cc:399:
> > FileGrowth(trusted_max_written_offset,
> > > 0))));
> > > On 2014/01/23 08:42:54, tzik wrote:
> > > > On 2014/01/22 20:47:27, bbudge wrote:
> > > > > We could probably change the FileIO class to know about append mode,
and
> > > > change
> > > > > NaClIPCAdapter to return 'bytes_written' so we can have an accurate
> value
> > > for
> > > > > this and can create a FileGrowth that has both of these values. Would
> that
> > > be
> > > > > helpful, or is this good enough for the quota system?
> > > > 
> > > > Ah, I didn't know we provide quota managed writable file descriptor to
> NaCl.
> > > > It has a separate mechanism to RequestOSFileHandle and is only for
trusted
> > > > plugins, right?
> > > > 
> > > > Yes, we should use QuotaReservation for them too.
> > > 
> > > The NaCl app gets a file descriptor that is backed by a NaClDescQuota in
the
> > > trusted part of the plugin process. That holds the real file descriptor /
> > handle
> > > and does the quota verification. It knows both the actual offset and
length
> of
> > > the write and can control how much is written. Here's the part that checks
> > > quota:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/lo...
> > > 
> > > So you'd change the FileIO class to have a write mode and a FileGrowth
> field,
> > > and change the Grow method.
> > 
> > Looking at them, it checks/consumes quota before writing, but it doesn't
> request
> > quota when it need.
> > Is it working now?
> 
> Yes, it should be working now. This layer just audits the message traffic
> between the plugin and host. It intercepts the quota reservation message reply
> to get the amount. The NaClDescQuota reports how much the plugin is consuming,
> which the auditing layer deducts from the reservation. This should mimic what
> happens in the proxy. We have to audit because the plugin could write with its
> file handle without running the proxy code.

Then, what I'm worried about is:
NaClDescQuota doesn't request quota even when the reservation is not enough to
write the data, and we don't allocate any initial quota for now.
So, every write operation through NaClDescQuota will probably fail.

Anyway, I believe this CL doesn't break existing NaClDesc code, and the fix for
it can be done in a separate CL.
Can I land this before that?

[bbudge, 2014-01-27 11:11:09]

On 2014/01/27 06:24:23, tzik wrote:
> On 2014/01/24 13:12:42, bbudge wrote:
> > On 2014/01/24 05:59:34, tzik wrote:
> > > On 2014/01/23 11:50:06, bbudge wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> > > > File ppapi/proxy/nacl_message_scanner.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/130053003/diff/270001/ppapi/proxy/nacl_messag...
> > > > ppapi/proxy/nacl_message_scanner.cc:399:
> > > FileGrowth(trusted_max_written_offset,
> > > > 0))));
> > > > On 2014/01/23 08:42:54, tzik wrote:
> > > > > On 2014/01/22 20:47:27, bbudge wrote:
> > > > > > We could probably change the FileIO class to know about append mode,
> and
> > > > > change
> > > > > > NaClIPCAdapter to return 'bytes_written' so we can have an accurate
> > value
> > > > for
> > > > > > this and can create a FileGrowth that has both of these values.
Would
> > that
> > > > be
> > > > > > helpful, or is this good enough for the quota system?
> > > > > 
> > > > > Ah, I didn't know we provide quota managed writable file descriptor to
> > NaCl.
> > > > > It has a separate mechanism to RequestOSFileHandle and is only for
> trusted
> > > > > plugins, right?
> > > > > 
> > > > > Yes, we should use QuotaReservation for them too.
> > > > 
> > > > The NaCl app gets a file descriptor that is backed by a NaClDescQuota in
> the
> > > > trusted part of the plugin process. That holds the real file descriptor
/
> > > handle
> > > > and does the quota verification. It knows both the actual offset and
> length
> > of
> > > > the write and can control how much is written. Here's the part that
checks
> > > > quota:
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/lo...
> > > > 
> > > > So you'd change the FileIO class to have a write mode and a FileGrowth
> > field,
> > > > and change the Grow method.
> > > 
> > > Looking at them, it checks/consumes quota before writing, but it doesn't
> > request
> > > quota when it need.
> > > Is it working now?
> > 
> > Yes, it should be working now. This layer just audits the message traffic
> > between the plugin and host. It intercepts the quota reservation message
reply
> > to get the amount. The NaClDescQuota reports how much the plugin is
consuming,
> > which the auditing layer deducts from the reservation. This should mimic
what
> > happens in the proxy. We have to audit because the plugin could write with
its
> > file handle without running the proxy code.
> 
> Then, what I'm worried about is:
> NaClDescQuota doesn't request quota even when the reservation is not enough to
> write the data, and we don't allocate any initial quota for now.
> So, every write operation through NaClDescQuota will probably fail.
> 
> Anyway, I believe this CL doesn't break existing NaClDesc code, and the fix
for
> it can be done in a separate CL.
> Can I land this before that?

If the plugin attempts to use its file descriptor to write directly, rather than
making Pepper calls, then what you say is true - the NaClDescQuota operation
will fail to get quota. This is actually what we want to happen. The plugin
should only use the FileIO API to write. That invokes our plugin code which does
reserve quota before doing the write. When the NaClDescQuota is finally used to
write (by our proxy code) the quota reservation should already exist.

I think it's OK to land this first. I can help with the follow on CL.

[bbudge, 2014-01-27 21:53:36]

LGTM with some comments

https://codereview.chromium.org/130053003/diff/400001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.h
(right):

https://codereview.chromium.org/130053003/diff/400001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:118:
const ppapi::FileGrowthMap& file_growth);
file_growths

https://codereview.chromium.org/130053003/diff/400001/ppapi/shared_impl/file_...
File ppapi/shared_impl/file_growth.h (right):

https://codereview.chromium.org/130053003/diff/400001/ppapi/shared_impl/file_...
ppapi/shared_impl/file_growth.h:26: PPAPI_SHARED_EXPORT FileGrowthMap
FileSizeMapToFileGrowthMap(
It might be a good idea to make it clear these are for testing:
e.g. FileSizeMapToFileGrowthMapForTesting

wdyt?

[tzik, 2014-01-28 06:12:49]

https://codereview.chromium.org/130053003/diff/400001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.h
(right):

https://codereview.chromium.org/130053003/diff/400001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.h:118:
const ppapi::FileGrowthMap& file_growth);
On 2014/01/27 21:53:37, bbudge wrote:
> file_growths

Done.

https://codereview.chromium.org/130053003/diff/400001/ppapi/shared_impl/file_...
File ppapi/shared_impl/file_growth.h (right):

https://codereview.chromium.org/130053003/diff/400001/ppapi/shared_impl/file_...
ppapi/shared_impl/file_growth.h:26: PPAPI_SHARED_EXPORT FileGrowthMap
FileSizeMapToFileGrowthMap(
On 2014/01/27 21:53:37, bbudge wrote:
> It might be a good idea to make it clear these are for testing:
> e.g. FileSizeMapToFileGrowthMapForTesting
> 
> wdyt?

Sounds nice. Done.

[commit-bot: I haz the power, 2014-01-28 06:13:05]

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

[commit-bot: I haz the power, 2014-01-28 18:35:43]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-01-28 18:38:03]

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

[commit-bot: I haz the power, 2014-01-28 21:19:07]

Change committed as 247504