PNaCl on-demand installs: Make a separate async IPC to check if PNaCl is installed

PNaCl on-demand installs: Make a separate async IPC to check if PNaCl is installed.

Previously, there was a single synchronous IPC that would
check if PNaCl is installed, then install if not installed,
as well as open the individual files read-only.

That sync IPC would block the UI if it needed to install PNaCl.

Instead separate checking from the opening a file read-only.
Have a separate IPC that is async, so that the UI is
not blocked, and progress events could eventually be sent.
Some basic plumbing of progress events is set up, but not
yet wired up to JS.

Order of events is:

CheckInstalled
* if Installed, reply w/ success
* if not Installed {
    send progress event indicated install start
    try install
    (may send more progress events)
    send progress of install success or failure
  }


BUG=252760
TEST=unit_tests --gtest_filter=NaClFileHostTest*  (and manual)

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

[jvoung (off chromium), 2013-07-23 16:59:54]

Do you guys want to take a look first if this is reasonable?  Waiting to merge
w/ some of that delegate refactoring.

https://codereview.chromium.org/19863003/diff/29001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_file_host_unittest.cc (right):

https://codereview.chromium.org/19863003/diff/29001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host_unittest.cc:64: virtual void
TryInstallPnacl(
Waiting for Yael's CL to land, which introduces this delegate method.

[Derek Schuff, 2013-07-23 21:39:29]

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:68:
RequestFirstInstall(cus, pci, installed);
maybe for another CL, but is there any reason this is a global function and not
a static method on PnaclComponentInstaller (or maybe in the pnacl namespace?)

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_file_host.cc (right):

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:89: const
nacl_file_host::InstallProgressCallback& progress_callback) {
maybe DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:92: if
(NaClBrowser::GetDelegate()->GetPnaclDirectory(&pnacl_dir) &&
actually, I think we may have to call NaClBrowser on the IO thread... although
it's probably ok as long as we ensure that the instance already exists, since
neither NaClBrowser nor the delegate should change... Does GetPnaclDirectory
have to be on the UI thread or a blocking thread, or does it matter?

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_file_host_unittest.cc (right):

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host_unittest.cc:85: public:
these can all be protected rather than public

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host_unittest.cc:224:
EXPECT_EQ(install_call_count(), 1);
EXPECT_EQs should have the expected value first, and the value under test second
(that's the "standard" way to do it so you get better error messages if you do
it that way)

https://codereview.chromium.org/19863003/diff/29002/ppapi/api/private/ppb_nac...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/19863003/diff/29002/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_private.idl:97: * or after an on-demand install is
attempted. Called back with PP_OK is
is->if

[jvoung (off chromium), 2013-07-31 21:41:07]

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:68:
RequestFirstInstall(cus, pci, installed);
On 2013/07/23 21:39:29, Derek Schuff wrote:
> maybe for another CL, but is there any reason this is a global function and
not
> a static method on PnaclComponentInstaller (or maybe in the pnacl namespace?)

Ah yes... moving out of global namespace in a separate CL.

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_file_host.cc (right):

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:89: const
nacl_file_host::InstallProgressCallback& progress_callback) {
On 2013/07/23 21:39:29, Derek Schuff wrote:
> maybe DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

Done.

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:92: if
(NaClBrowser::GetDelegate()->GetPnaclDirectory(&pnacl_dir) &&
On 2013/07/23 21:39:29, Derek Schuff wrote:
> actually, I think we may have to call NaClBrowser on the IO thread... although
> it's probably ok as long as we ensure that the instance already exists, since
> neither NaClBrowser nor the delegate should change...

NaClBrowser does get called in during early browser startup and that's when
SetDelegate() is done in the non-testing cases.  After that it's not set
anymore.  Not sure if that's early enough.


> Does GetPnaclDirectory
> have to be on the UI thread or a blocking thread, or does it matter?


The the non-test GetPnaclDirectory() implementation uses PathService.  That
claims to be safe to call from multiple threads and uses locks.

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_file_host_unittest.cc (right):

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host_unittest.cc:85: public:
On 2013/07/23 21:39:29, Derek Schuff wrote:
> these can all be protected rather than public

Done.

https://codereview.chromium.org/19863003/diff/29002/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host_unittest.cc:224:
EXPECT_EQ(install_call_count(), 1);
On 2013/07/23 21:39:29, Derek Schuff wrote:
> EXPECT_EQs should have the expected value first, and the value under test
second
> (that's the "standard" way to do it so you get better error messages if you do
> it that way)

Done.

https://codereview.chromium.org/19863003/diff/29002/ppapi/api/private/ppb_nac...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/19863003/diff/29002/ppapi/api/private/ppb_nac...
ppapi/api/private/ppb_nacl_private.idl:97: * or after an on-demand install is
attempted. Called back with PP_OK is
On 2013/07/23 21:39:29, Derek Schuff wrote:
> is->if

Done.

[Derek Schuff, 2013-07-31 22:08:12]

Make the commit message more descriptive (so that it makes sense out of context)
and LGTM

[jvoung (off chromium), 2013-08-01 00:50:07]

Thanks for the review!

dmichael, could you review too (and as pepper OWNER)?
jln, could you review nacl_host_messages?

[dmichael (off chromium), 2013-08-01 17:33:49]

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_file_host.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.cc:79: progress_callback.Run(0, -1);
It's not documented anywhere what this means ^^^

Might be more reason to use a little struct for the progress_callback, so your
intent is more obvious.

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.cc:107:
base::Bind(&SendNotInstalledProgress, done_callback, progress_callback));
You go through a lot of work to call Send() only from the IO thread...  are you
sure that's necessary? BrowserMessageFilter::Send already takes care of that for
you:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_file_host.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.h:30: typedef base::Callback<void(int64,
int64)> InstallProgressCallback;
I think you ought to document here what those int64 values are. Or to make it
self-documenting, make a little InstallProgress struct that contains the two
values as named attributes instead.

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.h:35: // If an request to install is
issued, then |progress_callback| is called
s/an/a

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_host_message_filter.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_host_message_filter.h:59: int64 total);
nit: I'm used to seeing message handler methods defined together and other stuff
(like ReplyEnsurePnacl... and SendProgress...) defined separately from the
message handlers. E.g., maybe these two should go down with
SyncReturnTemporaryFile and AsyncReturnTemporaryFile.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:157: if
(!io_message_loop_->BelongsToCurrentThread()) {
EnsurePnaclInstalled should always originate on the main renderer thread. I'd
rather you separate this in to two methods, the outer one that runs only on the
main thread and the internal one that only runs on the IO thread. Then you can
DCHECK your thread. That will make it more obvious what's going on and what your
thread expectations are.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:211:
pending_ensure_pnacl_requests_.clear();
So do CleanupEnsurePnaclRequests and the destructor always run on the IO thread?
Might be worth a DCHECK. (You might also have a look at ThreadChecker to make
some of the thread checking clear)

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.h:59: // Maps the
instance to an outstanding ensure pnacl installed request.
s/ensure pnacl installed/EnsurePnaclInstalled
might read better.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.h:80: IPC::Channel*
channel_;
^^^ it looks like channel_ should also only be accessed on the io thread.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:205: PP_CompletionCallback
callback) {
It might be good to DCHECK that you're on the main renderer thread, or do so in
InitializePnaclResourceHost, since it doesn't appear thread-safe.

[dmichael (off chromium), 2013-08-01 17:37:00]

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.h:61:
EnsurePnaclInstalledMap;
By the way... what happens if a particular plugin calls EnsurePnaclInstalledMap?
I presume it shouldn't happen, but it would be good to handle that in some way.

[jvoung (off chromium), 2013-08-01 23:14:06]

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_file_host.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.h:35: // If an request to install is
issued, then |progress_callback| is called
On 2013/08/01 17:33:49, dmichael wrote:
> s/an/a

Done.

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_host_message_filter.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_host_message_filter.h:59: int64 total);
On 2013/08/01 17:33:49, dmichael wrote:
> nit: I'm used to seeing message handler methods defined together and other
stuff
> (like ReplyEnsurePnacl... and SendProgress...) defined separately from the
> message handlers. E.g., maybe these two should go down with
> SyncReturnTemporaryFile and AsyncReturnTemporaryFile.

Done.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:157: if
(!io_message_loop_->BelongsToCurrentThread()) {
On 2013/08/01 17:33:49, dmichael wrote:
> EnsurePnaclInstalled should always originate on the main renderer thread. I'd
> rather you separate this in to two methods, the outer one that runs only on
the
> main thread and the internal one that only runs on the IO thread. Then you can
> DCHECK your thread. That will make it more obvious what's going on and what
your
> thread expectations are.


Separated the two halfs of the function and put in DCHECKS.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:211:
pending_ensure_pnacl_requests_.clear();
On 2013/08/01 17:33:49, dmichael wrote:
> So do CleanupEnsurePnaclRequests and the destructor always run on the IO
thread?
> Might be worth a DCHECK. (You might also have a look at ThreadChecker to make
> some of the thread checking clear)

Yeah, it also has to run on the IO thread. Added DCHECK.

I tried ThreadChecker, but I think the constructor doesn't run on the IO thread,
so it didn't work?

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.h:59: // Maps the
instance to an outstanding ensure pnacl installed request.
On 2013/08/01 17:33:49, dmichael wrote:
> s/ensure pnacl installed/EnsurePnaclInstalled
> might read better.

Done.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.h:61:
EnsurePnaclInstalledMap;
On 2013/08/01 17:37:00, dmichael wrote:
> By the way... what happens if a particular plugin calls
EnsurePnaclInstalledMap?
> I presume it shouldn't happen, but it would be good to handle that in some
way.

It could actually happen, if you load multiple PNaCl embeds at once.  It does
get a bit confusing if you allow all of them to proceed downstream and actually
try to install, because the code downstream rate-limits.  The second request
gets rejected.

I've changed the code to only send one request and queue the rest, and changed
the reply to broadcast the result to all queued requests.  That should be "okay"
for normal use.  It could be bad if an install was actually necessary, but the
first requester stops running.  Then the rest would be waiting indefinitely.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.h:80: IPC::Channel*
channel_;
On 2013/08/01 17:33:49, dmichael wrote:
> ^^^ it looks like channel_ should also only be accessed on the io thread.

Yes, I'll put it under the comment as well.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:205: PP_CompletionCallback
callback) {
On 2013/08/01 17:33:49, dmichael wrote:
> It might be good to DCHECK that you're on the main renderer thread, or do so
in
> InitializePnaclResourceHost, since it doesn't appear thread-safe.

Done. (within InitializePnaclResourceHost and shared with GetNexeFd)

[jvoung (off chromium), 2013-08-01 23:14:34]

Thanks for the review!  I looked into some of the parts, but will get to the
rest of them soon!

[dmichael (off chromium), 2013-08-02 17:24:03]

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:211:
pending_ensure_pnacl_requests_.clear();
On 2013/08/01 23:14:07, jvoung (cr) wrote:
> On 2013/08/01 17:33:49, dmichael wrote:
> > So do CleanupEnsurePnaclRequests and the destructor always run on the IO
> thread?
> > Might be worth a DCHECK. (You might also have a look at ThreadChecker to
make
> > some of the thread checking clear)
> 
> Yeah, it also has to run on the IO thread. Added DCHECK.
> 
> I tried ThreadChecker, but I think the constructor doesn't run on the IO
thread,
> so it didn't work?
FWIW, you can call DetachFromThread in that case.

[jvoung (off chromium), 2013-08-02 18:32:36]

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_file_host.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.cc:79: progress_callback.Run(0, -1);
On 2013/08/01 17:33:49, dmichael wrote:
> It's not documented anywhere what this means ^^^
> 
> Might be more reason to use a little struct for the progress_callback, so your
> intent is more obvious.

Done.

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.cc:107:
base::Bind(&SendNotInstalledProgress, done_callback, progress_callback));
On 2013/08/01 17:33:49, dmichael wrote:
> You go through a lot of work to call Send() only from the IO thread...  are
you
> sure that's necessary? BrowserMessageFilter::Send already takes care of that
for
> you:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...


Ah, yes forgot about that.  Otherwise, all the uses of NotifyRendererOfError
would have been wrong.

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_file_host.h (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_file_host.h:30: typedef base::Callback<void(int64,
int64)> InstallProgressCallback;
On 2013/08/01 17:33:49, dmichael wrote:
> I think you ought to document here what those int64 values are. Or to make it
> self-documenting, make a little InstallProgress struct that contains the two
> values as named attributes instead.

Done.

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):

https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:211:
pending_ensure_pnacl_requests_.clear();
On 2013/08/02 17:24:03, dmichael wrote:
> On 2013/08/01 23:14:07, jvoung (cr) wrote:
> > On 2013/08/01 17:33:49, dmichael wrote:
> > > So do CleanupEnsurePnaclRequests and the destructor always run on the IO
> > thread?
> > > Might be worth a DCHECK. (You might also have a look at ThreadChecker to
> make
> > > some of the thread checking clear)
> > 
> > Yeah, it also has to run on the IO thread. Added DCHECK.
> > 
> > I tried ThreadChecker, but I think the constructor doesn't run on the IO
> thread,
> > so it didn't work?
> FWIW, you can call DetachFromThread in that case.

Ah ok.  Any preference for using that or explicitly checking the IO thread?

[dmichael (off chromium), 2013-08-02 19:39:14]

On 2013/08/02 18:32:36, jvoung (cr) wrote:
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> File chrome/browser/nacl_host/nacl_file_host.cc (right):
> 
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> chrome/browser/nacl_host/nacl_file_host.cc:79: progress_callback.Run(0, -1);
> On 2013/08/01 17:33:49, dmichael wrote:
> > It's not documented anywhere what this means ^^^
> > 
> > Might be more reason to use a little struct for the progress_callback, so
your
> > intent is more obvious.
> 
> Done.
> 
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> chrome/browser/nacl_host/nacl_file_host.cc:107:
> base::Bind(&SendNotInstalledProgress, done_callback, progress_callback));
> On 2013/08/01 17:33:49, dmichael wrote:
> > You go through a lot of work to call Send() only from the IO thread...  are
> you
> > sure that's necessary? BrowserMessageFilter::Send already takes care of that
> for
> > you:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> 
> 
> Ah, yes forgot about that.  Otherwise, all the uses of NotifyRendererOfError
> would have been wrong.
> 
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> File chrome/browser/nacl_host/nacl_file_host.h (right):
> 
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> chrome/browser/nacl_host/nacl_file_host.h:30: typedef
base::Callback<void(int64,
> int64)> InstallProgressCallback;
> On 2013/08/01 17:33:49, dmichael wrote:
> > I think you ought to document here what those int64 values are. Or to make
it
> > self-documenting, make a little InstallProgress struct that contains the two
> > values as named attributes instead.
> 
> Done.
> 
>
https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
> File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):
> 
>
https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
> chrome/renderer/pepper/pnacl_translation_resource_host.cc:211:
> pending_ensure_pnacl_requests_.clear();
> On 2013/08/02 17:24:03, dmichael wrote:
> > On 2013/08/01 23:14:07, jvoung (cr) wrote:
> > > On 2013/08/01 17:33:49, dmichael wrote:
> > > > So do CleanupEnsurePnaclRequests and the destructor always run on the IO
> > > thread?
> > > > Might be worth a DCHECK. (You might also have a look at ThreadChecker to
> > make
> > > > some of the thread checking clear)
> > > 
> > > Yeah, it also has to run on the IO thread. Added DCHECK.
> > > 
> > > I tried ThreadChecker, but I think the constructor doesn't run on the IO
> > thread,
> > > so it didn't work?
> > FWIW, you can call DetachFromThread in that case.
> 
> Ah ok.  Any preference for using that or explicitly checking the IO thread?
I don't really care. Do whatever you think is clearest.

[jvoung (off chromium), 2013-08-02 21:17:14]

On 2013/08/02 19:39:14, dmichael wrote:
> On 2013/08/02 18:32:36, jvoung (cr) wrote:
> >
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> > File chrome/browser/nacl_host/nacl_file_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> > chrome/browser/nacl_host/nacl_file_host.cc:79: progress_callback.Run(0, -1);
> > On 2013/08/01 17:33:49, dmichael wrote:
> > > It's not documented anywhere what this means ^^^
> > > 
> > > Might be more reason to use a little struct for the progress_callback, so
> your
> > > intent is more obvious.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> > chrome/browser/nacl_host/nacl_file_host.cc:107:
> > base::Bind(&SendNotInstalledProgress, done_callback, progress_callback));
> > On 2013/08/01 17:33:49, dmichael wrote:
> > > You go through a lot of work to call Send() only from the IO thread... 
are
> > you
> > > sure that's necessary? BrowserMessageFilter::Send already takes care of
that
> > for
> > > you:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> > 
> > 
> > Ah, yes forgot about that.  Otherwise, all the uses of NotifyRendererOfError
> > would have been wrong.
> > 
> >
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> > File chrome/browser/nacl_host/nacl_file_host.h (right):
> > 
> >
>
https://codereview.chromium.org/19863003/diff/103001/chrome/browser/nacl_host...
> > chrome/browser/nacl_host/nacl_file_host.h:30: typedef
> base::Callback<void(int64,
> > int64)> InstallProgressCallback;
> > On 2013/08/01 17:33:49, dmichael wrote:
> > > I think you ought to document here what those int64 values are. Or to make
> it
> > > self-documenting, make a little InstallProgress struct that contains the
two
> > > values as named attributes instead.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
> > File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/19863003/diff/103001/chrome/renderer/pepper/p...
> > chrome/renderer/pepper/pnacl_translation_resource_host.cc:211:
> > pending_ensure_pnacl_requests_.clear();
> > On 2013/08/02 17:24:03, dmichael wrote:
> > > On 2013/08/01 23:14:07, jvoung (cr) wrote:
> > > > On 2013/08/01 17:33:49, dmichael wrote:
> > > > > So do CleanupEnsurePnaclRequests and the destructor always run on the
IO
> > > > thread?
> > > > > Might be worth a DCHECK. (You might also have a look at ThreadChecker
to
> > > make
> > > > > some of the thread checking clear)
> > > > 
> > > > Yeah, it also has to run on the IO thread. Added DCHECK.
> > > > 
> > > > I tried ThreadChecker, but I think the constructor doesn't run on the IO
> > > thread,
> > > > so it didn't work?
> > > FWIW, you can call DetachFromThread in that case.
> > 
> > Ah ok.  Any preference for using that or explicitly checking the IO thread?
> I don't really care. Do whatever you think is clearest.


Well, I think explicitly checking that it is on the IO thread would match the
comment in the header, and give a better sense of the threads involved.

[Derek Schuff, 2013-08-02 21:24:56]

On 2013/08/02 21:17:14, jvoung (cr) wrote:
> Well, I think explicitly checking that it is on the IO thread would match the
> comment in the header, and give a better sense of the threads involved.

The one benefit of ThreadChecker (or using an explicit io_message_loop_ member)
is that it also works in a unittest with a fake IO message loop. But this looks
fine for now.

[dmichael (off chromium), 2013-08-05 22:38:11]

lgtm

[jvoung (off chromium), 2013-08-05 22:50:17]

Thanks dmichael.

jln can you take a look, or +jschuh?

[jln (very slow on Chromium), 2013-08-05 23:01:55]

On 2013/08/05 22:50:17, jvoung (cr) wrote:
> Thanks dmichael.
> 
> jln can you take a look, or +jschuh?

I'm on it!

[jln (very slow on Chromium), 2013-08-05 23:58:28]

*_messages.h lgtm, but I would prefer NaClViewMsg_EnsurePnaclInstalledProgress
to be added in another CL when the actual implementation will happen.

Not a must as it's fairly low risk and in the renderer.

https://chromiumcodereview.appspot.com/19863003/diff/131001/chrome/renderer/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):

https://chromiumcodereview.appspot.com/19863003/diff/131001/chrome/renderer/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:224: void
PnaclTranslationResourceHost::OnEnsurePnaclInstalledProgress(
1. This is not a great way to do things as far as security is concerned, since
this new handler will completely bypass the IPC review process.

I don't particularly mind, especially since this is in a renderer. But keep in
mind that it's usually preferable to not create IPCs for future functions.

2. Could you check state here ? i.e. keep track of the state (the fact that
you've sent a query to the browser) somehow.

It's a good thing to catch attackers sending IPCs at an expected time. Again,
since this is in the renderer, it's not particularly important.

[jvoung (off chromium), 2013-08-06 01:11:13]

https://chromiumcodereview.appspot.com/19863003/diff/131001/chrome/renderer/p...
File chrome/renderer/pepper/pnacl_translation_resource_host.cc (right):

https://chromiumcodereview.appspot.com/19863003/diff/131001/chrome/renderer/p...
chrome/renderer/pepper/pnacl_translation_resource_host.cc:224: void
PnaclTranslationResourceHost::OnEnsurePnaclInstalledProgress(
On 2013/08/05 23:58:28, Julien Tinnes wrote:
> 1. This is not a great way to do things as far as security is concerned, since
> this new handler will completely bypass the IPC review process.
> 
> I don't particularly mind, especially since this is in a renderer. But keep in
> mind that it's usually preferable to not create IPCs for future functions.
> 
> 2. Could you check state here ? i.e. keep track of the state (the fact that
> you've sent a query to the browser) somehow.
> 
> It's a good thing to catch attackers sending IPCs at an expected time. Again,
> since this is in the renderer, it's not particularly important.


Ok, I'll take it out for now, and stub out earlier.  For (2), I could change the
'pending_ensure_pnacl_requests_' back from a vector to a map so that it could
check that the instance is actually in that map.  I'm not sure how hard it is to
guess the PP_Instance values though.

[commit-bot: I haz the power, 2013-08-06 03:10:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@chromium.org/19863003/165001

[commit-bot: I haz the power, 2013-08-06 07:46:20]

Change committed as 215844