Do PPB_FileIO Query and Read in the plugin process.

Move PPB_FileIO Query and Read implementation from the renderer to the plugin process.
This sends the file descriptor to the plugin side and moves some FileIO
implementation from the renderer to the plugin, using that file descriptor.

BUG=194304
R=dmichael@chromium.org, sky@chromium.org, teravest@chromium.org

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

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

[bbudge, 2013-07-11 20:59:48]

Ignore everything in base/. It's in a separate CL but is required to build this.

[dmichael (off chromium), 2013-07-12 15:46:24]

Sorry for being slow, there was an office picnic yesterday.

+teravest@, I think he'll be a good reviewer for this.

I haven't looked much yet, but could you update the CL description? This is now
only doing read-only stuff, right?

[teravest, 2013-07-12 16:07:50]

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
content/renderer/pepper/pepper_file_io_host.cc:423:
renderer_ppapi_host_->ShareHandleWithRemote(file_, false);
What's your plan for preventing writes to file_ in the plugin?
I don't know much about NaClDesc instances, but I think there are only two
options:
  1. Open two files (one with writes, one that's read-only), and send the
read-only file descriptor to the plugin.
  2. Do some management with NaClDesc to prevent writes to the file descriptor
on the plugin side.

Dave pointed out that we should be careful to do the right thing with trusted
plugins, so maybe we can't rely on managing file descriptor permissions with
NaClDesc?

[bbudge, 2013-07-12 17:19:14]

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
content/renderer/pepper/pepper_file_io_host.cc:423:
renderer_ppapi_host_->ShareHandleWithRemote(file_, false);
Good question! I hadn't dealt with this since I originally intended to implement
plugin-side write in this patch.

I think I'll have to open two file descriptors, share the read-only one, and
close it on the host side.

[bbudge, 2013-07-12 18:15:18]

+Justin for IPC and overall security review.

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
content/renderer/pepper/pepper_file_io_host.cc:423:
renderer_ppapi_host_->ShareHandleWithRemote(file_, false);
I looked through the code and the NaClDesc does check the flags before writing
with the file descriptor. So although the plugin-side fd has write capability,
if we eliminate these flags on the host side, it shouldn't be possible for
untrusted code to write to it.

[teravest, 2013-07-15 16:25:12]

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/18063005/diff/5655612935372800/content/render...
content/renderer/pepper/pepper_file_io_host.cc:423:
renderer_ppapi_host_->ShareHandleWithRemote(file_, false);
On 2013/07/12 18:15:18, bbudge1 wrote:
> I looked through the code and the NaClDesc does check the flags before writing
> with the file descriptor. So although the plugin-side fd has write capability,
> if we eliminate these flags on the host side, it shouldn't be possible for
> untrusted code to write to it.

Cool. I don't see where that's happening now; are you going to update this patch
to do that?

[bbudge, 2013-07-15 17:06:51]

I added a comment where the write flags are cleared.

For some reason Query fails on Windows / NaCl so I still need to figure that
out. I've spent a few days trying to get a good Windows checkout. Hopefully
today!

https://codereview.chromium.org/18063005/diff/76001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/18063005/diff/76001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.cc:431: open_flags_ &
~(PP_FILEOPENFLAG_WRITE | PP_FILEOPENFLAG_APPEND);
This should clear the flags that are sent to the plugin process and intercepted
in the trusted part (nacl_ipc_adapter.cc).

[dmichael (off chromium), 2013-07-15 17:19:41]

On Mon, Jul 15, 2013 at 11:06 AM, <bbudge@chromium.org> wrote:

> I added a comment where the write flags are cleared.
>
> For some reason Query fails on Windows / NaCl so I still need to figure
> that
> out. I've spent a few days trying to get a good Windows checkout. Hopefully
> today!
>
This may be a sandbox issue. It would be worth trying with --no-sandbox
just to help you determine if that is the problem.


>
>
> https://codereview.chromium.**org/18063005/diff/76001/**
>
content/renderer/pepper/**pepper_file_io_host.cc<https://codereview.chromium.org/18063005/diff/76001/content/renderer/pepper/pepper_file_io_host.cc>
> File content/renderer/pepper/**pepper_file_io_host.cc (right):
>
> https://codereview.chromium.**org/18063005/diff/76001/**
>
content/renderer/pepper/**pepper_file_io_host.cc#**newcode431<https://codereview.chromium.org/18063005/diff/76001/content/renderer/pepper/pepper_file_io_host.cc#newcode431>
> content/renderer/pepper/**pepper_file_io_host.cc:431: open_flags_ &
> ~(PP_FILEOPENFLAG_WRITE | PP_FILEOPENFLAG_APPEND);
> This should clear the flags that are sent to the plugin process and
> intercepted in the trusted part (nacl_ipc_adapter.cc).
>
>
https://codereview.chromium.**org/18063005/<https://codereview.chromium.org/1...
>

[jschuh, 2013-07-16 00:05:47]

IPC security lgtm. It looks okay, but Pepper code is always really ahrd for me
to follow.

[bbudge, 2013-07-17 21:46:33]

I tracked down the cause of the failures. It looks like the NaClDesc is using
Windows' POSIX porting code, specifically _fstat64. This returns odd values for
very small Unix Epoch time values. Before, the test must have passed because we
used the Windows API and did our own time conversion. At any rate, the NaCl code
path isn't consistent with the old Chrome platform_file_win one so the
last_modified_time value is wrong. I could try to rewrite the NaClDesc file to
use Windows APIs, but that seems like a big change, so I instead finesse the
test by using larger values.

[teravest, 2013-07-18 19:00:35]

lgtm

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:44: typedef
scoped_ptr<scoped_refptr<FileIOResource> > PassedRef;
This is funky, but I couldn't quickly think of a better way.

[dmichael (off chromium), 2013-07-18 20:28:09]

https://codereview.chromium.org/18063005/diff/110003/chrome/nacl/nacl_ipc_ada...
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/18063005/diff/110003/chrome/nacl/nacl_ipc_ada...
chrome/nacl/nacl_ipc_adapter.cc:441: // have write capabilities, and we don't
want the plugin to be able to
"may have write capabilities"...  we intend not to do that, right? Maybe it
would be better to say "should not have write capabilities"?

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:98: return PP_ERROR_FAILED;
^^^ Seems like you could add NOTREACHED() before the return

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:131: base::Bind(&QueryCallback,
base::Passed(&passed_ref),
Could you just use RunWhileLocked?
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/shared_impl/...

Then I don't think you need QueryCallback, and I also think you could make
DoQueryCallback private (same comment for the others).

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:367: // End the operation now. The callback may
perform another file operation.
This comment and similar ones aren't quite right. state_manager_ actually has a
count of operations. There might still be a read pending, so the pending
operation count is still non-zero, so the state manager still will not allow a
write (for example).

[bbudge, 2013-07-18 21:28:16]

https://codereview.chromium.org/18063005/diff/110003/chrome/nacl/nacl_ipc_ada...
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/18063005/diff/110003/chrome/nacl/nacl_ipc_ada...
chrome/nacl/nacl_ipc_adapter.cc:441: // have write capabilities, and we don't
want the plugin to be able to
It's non-trivial to create and share a fd with different capabilities in the
host. So we may return an fd with write capabilities, and we count on the
NaClDesc to fail before making the system call. This check happens in trusted
code so should be safe, as long as we're sure the write flags have been cleared
on the host side. We can't do that here because it would break the trusted and
private APIs.

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:44: typedef
scoped_ptr<scoped_refptr<FileIOResource> > PassedRef;
On 2013/07/18 19:00:36, teravest wrote:
> This is funky, but I couldn't quickly think of a better way.

If you code search on 'scoped_ptr<scoped_refptr' you will find a few precedents.
It *is* funky though.

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:98: return PP_ERROR_FAILED;
On 2013/07/18 20:28:09, dmichael wrote:
> ^^^ Seems like you could add NOTREACHED() before the return

Done.

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:131: base::Bind(&QueryCallback,
base::Passed(&passed_ref),
I tried that but couldn't get it to compile. The problem is that these are
callbacks, and FileUtilProxy adds a few parameters to them, so I can't create a
Closure. Ideas?

If I can't use RunWhileLocked, I could move some definitions into the header and
make the methods private, by declaring these as friend functions. Do you think
it's worth it?

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:367: // End the operation now. The callback may
perform another file operation.
I'm not sure I understand but I modified the comments to try to be a little
clearer.

[dmichael (off chromium), 2013-07-18 23:21:47]

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:131: base::Bind(&QueryCallback,
base::Passed(&passed_ref),
On 2013/07/18 21:28:16, bbudge1 wrote:
> I tried that but couldn't get it to compile. The problem is that these are
> callbacks, and FileUtilProxy adds a few parameters to them, so I can't create
a
> Closure. Ideas?
Oh, right, sorry. I started poking this on a local branch to come up with a
general solution. In the mean time, maybe you can pass a bound callback to
QueryCallback instead of the PassedRef. That would allow you to make
OnQueryCompleted private, and would eliminate having to use
scoped_ptr<scoped_ref_ptr.

I.e., QueryCallback would look something like this:
void QueryCallback(
    const base::Callback<void(base::PlatformFileError,
                              base::PlatformFileInfo)>& cb,
    base::PlatformFileError error_code,
    const base::PlatformFileInfo& file_info) {
  ProxyAutoLock lock;
  cb->Run(error_code, file_info);
}

And you have two Binds here, something like:
base::Bind(
    &QueryCallback,
    base::Bind(&FileIOResource::OnQueryCompleted,
               this,
               callback,
               info))

Does that make sense?

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:367: // End the operation now. The callback may
perform another file operation.
On 2013/07/18 21:28:16, bbudge1 wrote:
> I'm not sure I understand but I modified the comments to try to be a little
> clearer.
Reads can be done concurrently with other reads, but not with any other kinds of
operations. Similarly, writes can be done concurrently with other writes.

So if the plugin has 3 Reads pending, when the first one returns, the plugin
still is not allowed to do any operations other than Read. It just decrements
the count of pending operations (in this case, the pending operations are reads)
to 2. But the state_manager_ won't allow other kinds of operations unless the
count goes to zero.

Does that help? Your new comment looks fine.

[bbudge, 2013-07-19 01:16:56]

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:131: base::Bind(&QueryCallback,
base::Passed(&passed_ref),
This does compile, but unfortunately the outermost callback owns the inner one,
which holds a ref to the FileIOResource, so the FileIOResource destructor still
runs outside of the proxy lock when the callback holds the last ref. The problem
is that base::Callback holds onto its arguments for its lifetime, except for
scoped_ptr's. When I run the FileIO tests, the abort tests still fail when they
hit the locked assert.

Am I missing something?

[dmichael (off chromium), 2013-07-19 02:28:31]

On 2013/07/19 01:16:56, bbudge1 wrote:
>
https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
> File ppapi/proxy/file_io_resource.cc (right):
> 
>
https://codereview.chromium.org/18063005/diff/110003/ppapi/proxy/file_io_reso...
> ppapi/proxy/file_io_resource.cc:131: base::Bind(&QueryCallback,
> base::Passed(&passed_ref),
> This does compile, but unfortunately the outermost callback owns the inner
one,
> which holds a ref to the FileIOResource, so the FileIOResource destructor
still
> runs outside of the proxy lock when the callback holds the last ref. The
problem
> is that base::Callback holds onto its arguments for its lifetime, except for
> scoped_ptr's. When I run the FileIO tests, the abort tests still fail when
they
> hit the locked assert.
> 
> Am I missing something?

Ugh, sorry for wasting your time. I'm going to have to think about it some
more...  RunWhileLocked probably has the same bug.

lgtm

[bbudge, 2013-07-20 12:52:52]

I'm going to wait until you land your RunWhileLocked change.

[dmichael (off chromium), 2013-07-25 17:42:46]

https://codereview.chromium.org/18063005/diff/167001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/167001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:313: file_io->OnReadComplete(callback,
array_output, error_code, data, bytes_read);
It looks like this stuff snuck back in. Was that on purpose?

[bbudge, 2013-07-25 17:56:24]

https://codereview.chromium.org/18063005/diff/167001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/167001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:313: file_io->OnReadComplete(callback,
array_output, error_code, data, bytes_read);
No. I'm not sure what happened here.

[dmichael (off chromium), 2013-07-25 21:39:54]

lgtm without the test_globals.* change

https://codereview.chromium.org/18063005/diff/189001/ppapi/shared_impl/test_g...
File ppapi/shared_impl/test_globals.h (right):

https://codereview.chromium.org/18063005/diff/189001/ppapi/shared_impl/test_g...
ppapi/shared_impl/test_globals.h:52: void set_proxy_lock(base::Lock* lock) {
lock_ = lock; }
I think this is a holdover from an old version of my patch. We don't need to
change TestGlobals yet (though my next CL will change it in a different way)

[bbudge, 2013-07-25 22:12:07]

https://codereview.chromium.org/18063005/diff/189001/ppapi/shared_impl/test_g...
File ppapi/shared_impl/test_globals.h (right):

https://codereview.chromium.org/18063005/diff/189001/ppapi/shared_impl/test_g...
ppapi/shared_impl/test_globals.h:52: void set_proxy_lock(base::Lock* lock) {
lock_ = lock; }
On 2013/07/25 21:39:54, dmichael wrote:
> I think this is a holdover from an old version of my patch. We don't need to
> change TestGlobals yet (though my next CL will change it in a different way)

Done.

[commit-bot: I haz the power, 2013-07-25 22:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/193001

[commit-bot: I haz the power, 2013-07-25 22:35:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/193001

[commit-bot: I haz the power, 2013-07-25 23:06:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/193001

[commit-bot: I haz the power, 2013-07-26 03:37:57]

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

[commit-bot: I haz the power, 2013-07-26 03:44:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/219001

[commit-bot: I haz the power, 2013-07-26 06:37:52]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-26 13:17:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/219001

[commit-bot: I haz the power, 2013-07-26 13:40:21]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bbudge, 2013-07-26 15:01:03]

+sky, +jhawkins for OWNERS review for chrome/nacl/nacl_ipc_adapter.cc. Just a
comment change.

[sky, 2013-07-26 15:40:55]

Rubber stamp LGTM feel free to TBR comment only changes.

[bbudge, 2013-07-26 16:49:50]

Committed patchset #23 manually as r213933 (presubmit successful).

[bbudge, 2013-08-01 21:09:16]

This is a rough version of what it will take to make this work for all the cases
right now. It depends on a separate TrackedCallback::IsBlocking CL.

I need to add some comments explaining why two impls are needed, perhaps with
some TODOs.

[bbudge, 2013-08-02 13:53:56]

Added some comments to explain why we can't use FileUtilProxy in the blocking
callback case. PTAL

[dmichael (off chromium), 2013-08-02 15:44:28]

https://codereview.chromium.org/18063005/diff/251001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/251001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:35: // block the calling thread, which will
cause FileUtilProxy to hang. Instead,
how about instead of "which will cause FileUtilProxy to hang"
...
"and so it can not receive the callback from FileUtilProxy."
or something (just to add more detail to why there's a hang)

https://codereview.chromium.org/18063005/diff/251001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:56: scoped_ptr<char[]> buffer(new
char[bytes_to_read]);
We shouldn't have to make a copy for the "Read" case. Maybe you can pass the
ArrayOutput here and do the read directly in to it?

In fact...  I hadn't realized that FileUtilProxy forces us to make a copy! We
probably shouldn't be using FileUtilProxy at all :-(. I think you can either:
1) Pass is_blocking along to DoRead, and if it's non-blocking, PostTask to the
main thread.
or...
2) Don't bother with is_blocking(). Check in here if ProxyLock is NULL. If so,
you know you need to PostTask to the main thread.

I think I like 2 better than 1. I think it's clearer, and gives you a good place
to stick a "TODO" to get rid of the special case. Maybe more importantly, if we
do (1), for any non-blocking callback that is *not* coming from the main thread,
we'd be bouncing to the main thread only to then bounce it to the real thread.

[bbudge, 2013-08-02 20:56:44]

https://codereview.chromium.org/18063005/diff/251001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/251001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:35: // block the calling thread, which will
cause FileUtilProxy to hang. Instead,
n/a now that we've eliminated FileUtilProxy.

https://codereview.chromium.org/18063005/diff/251001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:56: scoped_ptr<char[]> buffer(new
char[bytes_to_read]);
Looks like we can't avoid the copy due to Abort semantics. Either we hold the
proxy lock for the entire file operation, or we copy.

I did eliminate FileUtilProxy, so will remove it from the IRT build in another
CL.

Note: we're no longer using is_blocking(), so I'll revert it too.

[dmichael (off chromium), 2013-08-02 22:28:16]

lgtm, thanks for persevering

https://codereview.chromium.org/18063005/diff/260001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/260001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:76: Bind(callback, error,
base::Owned(buffer.release()), bytes_read));
nit: base::Passed is a slightly cleaner way to pass a scoped_ptr

[bbudge, 2013-08-03 02:20:27]

https://codereview.chromium.org/18063005/diff/260001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/260001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:76: Bind(callback, error,
base::Owned(buffer.release()), bytes_read));
Can't do it with RunWhileLocked yet, due to scoped_ptr copying.

[bbudge, 2013-08-06 20:58:05]

https://codereview.chromium.org/18063005/diff/270001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/270001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:72: callback.Run(error, buffer.get(),
bytes_read);
David: is it safe to pass the buffer this way? callback will wake the original
thread, so will this go out of scope during the copying?

[bbudge, 2013-08-07 19:05:38]

I decided to explore optimizing only the blocking callback case. It looks much
simpler and faster. I still need to comment on some of the subtleties, and I'd
like to only send the file descriptor to the plugin if Open is called with a
blocking callback. That way we don't penalize everybody with this extra file
handle.

I think this approach has multiple advantages over trying to optimize
non-blocking callbacks. This does require me to re-land the change to make
TrackedCallback::is_blocking() public.

https://codereview.chromium.org/18063005/diff/282001/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/282001/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:33: }
We could eliminate this and the file thread if we knew close() wouldn't block
slow us down. What do you think?

[bbudge, 2013-08-07 20:33:56]

OK I think this is ready. Thanks again for all your reviewing.

[bbudge, 2013-08-07 20:38:21]

On 2013/08/07 20:33:56, bbudge1 wrote:
> OK I think this is ready. Thanks again for all your reviewing.

Also, I will rebase after the CL to TrackedCallback lands.

[dmichael (off chromium), 2013-08-08 17:44:43]

It kind of looks like you haven't added the heuristic of only getting the FD for
blocking Open()? FWIW, I prefer it without that heuristic...  simpler to
explain, covers more usages. (E.g., I can imagine scenarios where you'd fire off
a bunch of Opens asynchronously in parallel, but still do blocking Reads and
Writes.)

And...  how do you feel about still doing the asynchronous case in the plugin in
the future? I think this is a good first step, and I'm glad it's simpler. But I
still would ultimately like to get us to doing all Reads in the plugin and all
Writes in the plugin.

https://chromiumcodereview.appspot.com/18063005/diff/282001/ppapi/proxy/file_...
File ppapi/proxy/file_io_resource.cc (right):

https://chromiumcodereview.appspot.com/18063005/diff/282001/ppapi/proxy/file_...
ppapi/proxy/file_io_resource.cc:33: }
On 2013/08/07 19:05:38, bbudge1 wrote:
> We could eliminate this and the file thread if we knew close() wouldn't block
> slow us down. What do you think?
I'm not sure. I'd look around at how ClosePlatformFile is typically used in
other parts of Chromium.

https://chromiumcodereview.appspot.com/18063005/diff/284002/ppapi/proxy/file_...
File ppapi/proxy/file_io_resource.cc (right):

https://chromiumcodereview.appspot.com/18063005/diff/284002/ppapi/proxy/file_...
ppapi/proxy/file_io_resource.cc:285: scoped_ptr<char[]> buffer(new
char[bytes_to_read]);
It would probably be better to do the allocation after you drop the lock, just
to minimize the time you're holding the lock.

https://chromiumcodereview.appspot.com/18063005/diff/284002/ppapi/proxy/file_...
ppapi/proxy/file_io_resource.cc:291: result = (bytes_read >= 0) ? bytes_read :
PP_ERROR_FAILED;
I may be reading it wrong, but it looks in the host like 0 is a valid param for
bytes_to_read, and therefore a valid response. I think you might want
(bytes_read > 0)?

[bbudge, 2013-08-08 18:44:31]

I decided to skip the 'minimize fd's' optimization for all the reasons you
state.

I intend to pursue the asynchronous case very soon. I think I see a way to do it
using thread hopping, TrackedCallback::CompletionTask) and having some extra
fields on the resource to hold results. The in-process case complicates things a
bit.

Not giving up!

https://codereview.chromium.org/18063005/diff/284002/ppapi/proxy/file_io_reso...
File ppapi/proxy/file_io_resource.cc (right):

https://codereview.chromium.org/18063005/diff/284002/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:285: scoped_ptr<char[]> buffer(new
char[bytes_to_read]);
On 2013/08/08 17:44:44, dmichael wrote:
> It would probably be better to do the allocation after you drop the lock, just
> to minimize the time you're holding the lock.

Done.

https://codereview.chromium.org/18063005/diff/284002/ppapi/proxy/file_io_reso...
ppapi/proxy/file_io_resource.cc:291: result = (bytes_read >= 0) ? bytes_read :
PP_ERROR_FAILED;
On 2013/08/08 17:44:44, dmichael wrote:
> I may be reading it wrong, but it looks in the host like 0 is a valid param
for
> bytes_to_read, and therefore a valid response. I think you might want
> (bytes_read > 0)?

That would cause a result of 0 to return PP_ERROR_FAILED. I switched this around
to hopefully make it clearer.

[teravest, 2013-08-08 19:31:35]

lgtm

[dmichael (off chromium), 2013-08-08 19:51:38]

Thanks, lgtm again

https://chromiumcodereview.appspot.com/18063005/diff/284002/ppapi/proxy/file_...
File ppapi/proxy/file_io_resource.cc (right):

https://chromiumcodereview.appspot.com/18063005/diff/284002/ppapi/proxy/file_...
ppapi/proxy/file_io_resource.cc:291: result = (bytes_read >= 0) ? bytes_read :
PP_ERROR_FAILED;
On 2013/08/08 18:44:31, bbudge1 wrote:
> On 2013/08/08 17:44:44, dmichael wrote:
> > I may be reading it wrong, but it looks in the host like 0 is a valid param
> for
> > bytes_to_read, and therefore a valid response. I think you might want
> > (bytes_read > 0)?
> 
> That would cause a result of 0 to return PP_ERROR_FAILED. I switched this
around
> to hopefully make it clearer.
My bad, thanks

[commit-bot: I haz the power, 2013-08-08 20:05:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/311001

[commit-bot: I haz the power, 2013-08-08 22:12:57]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-08 23:02:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/311001

[commit-bot: I haz the power, 2013-08-09 00:35:05]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-09 01:18:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/18063005/311001

[commit-bot: I haz the power, 2013-08-09 04:18:10]

Change committed as 216569