NaCl: Simpler validation for main nexe.

NaCl: Simpler validation for main nexe.

This change gets nexe file identity while doing work in the browser process,
preventing an unnecessary plugin->browser IPC round, and simplifying this
codepath in the process.

BUG=394130
TEST=NonSfiPackagedAppTest.SuccessfulLoad covers the Non-SFI case.
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32

Committed: https://crrev.com/c6b3ad13c7dcf0b4359d75798463c2677f9c8e78
Cr-Commit-Position: refs/heads/master@{#304965}

[teravest, 2014-10-13 18:56:01]

Patchset #1 (id:1) has been deleted

[teravest, 2014-10-14 15:56:05]

Patchset #1 (id:20001) has been deleted

[teravest, 2014-10-14 17:12:46]

teravest@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2014-10-14 17:37:24]

https://codereview.chromium.org/572973002/diff/200001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:910: void
ClosePlatformFile(base::PlatformFile file) {
Shouldn't this be in an anon namespace?  Doesn't base/ provide a function to
close a PlatformFile?

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
File components/nacl/common/nacl_messages.h (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
components/nacl/common/nacl_messages.h:94:
IPC_SYNC_MESSAGE_CONTROL2_2(NaClProcessMsg_ResolveFileToken,
You can remove this, right?

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
components/nacl/common/nacl_messages.h:100:
IPC_MESSAGE_CONTROL2(NaClProcessMsg_ResolveFileTokenAsync,
Should the "Async" suffix be removed from the name of this, since only the async
version remains?

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
components/nacl/common/nacl_types.h:66: base::FilePath nexe_file_path;
Could you comment that this is used only as a key for validation caching? 
Otherwise it looks strange that a filename would be passed to a sandboxed
process that can't open files by name.

Ideally this would be named something more abstract like
"nexe_validation_cache_key", but I suppose the name should match up with the
NaClDescCreateWithFilePathMetadata() API.

So how about "nexe_file_path_metadata"?

https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:36: #include
"native_client/src/trusted/validator/rich_file_info.h"
This isn't needed, is it?

https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:437: if (!params.nexe_file_path.empty())
{
Hmm, if this check were omitted, would this be insecure (or less secure)?  Would
we accidentally treat all nexes as having the same identity?  (Or would the
identity be diluted?  We'd include fstat() metadata in the identity but not the
filename.)

How about changing NaClDescCreateWithFilePathMetadata() so that either:

 * it rejects an empty filename;

 * better: when given an empty filename, it falls back to the non-caching
version (NaClDescIoDescFromDescAllocCtor() as below).  Then we wouldn't need 2
code paths in this Chromium code.

[teravest, 2014-10-14 18:15:01]

https://codereview.chromium.org/572973002/diff/200001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:910: void
ClosePlatformFile(base::PlatformFile file) {
On 2014/10/14 17:37:24, Mark Seaborn wrote:
> Shouldn't this be in an anon namespace?  Doesn't base/ provide a function to
> close a PlatformFile?

I've moved this to an anonymous namespace and changed to pass a base::File
instead of having a routine to close a PlatformFile.

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
File components/nacl/common/nacl_messages.h (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
components/nacl/common/nacl_messages.h:94:
IPC_SYNC_MESSAGE_CONTROL2_2(NaClProcessMsg_ResolveFileToken,
On 2014/10/14 17:37:24, Mark Seaborn wrote:
> You can remove this, right?

Done.

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
components/nacl/common/nacl_messages.h:100:
IPC_MESSAGE_CONTROL2(NaClProcessMsg_ResolveFileTokenAsync,
On 2014/10/14 17:37:24, Mark Seaborn wrote:
> Should the "Async" suffix be removed from the name of this, since only the
async
> version remains?

Done.

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
components/nacl/common/nacl_types.h:66: base::FilePath nexe_file_path;
On 2014/10/14 17:37:24, Mark Seaborn wrote:
> Could you comment that this is used only as a key for validation caching? 
> Otherwise it looks strange that a filename would be passed to a sandboxed
> process that can't open files by name.
> 
> Ideally this would be named something more abstract like
> "nexe_validation_cache_key", but I suppose the name should match up with the
> NaClDescCreateWithFilePathMetadata() API.
> 
> So how about "nexe_file_path_metadata"?

Done.

https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:36: #include
"native_client/src/trusted/validator/rich_file_info.h"
On 2014/10/14 17:37:24, Mark Seaborn wrote:
> This isn't needed, is it?

Nope, removed.

https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:437: if (!params.nexe_file_path.empty())
{
On 2014/10/14 17:37:24, Mark Seaborn wrote:
> Hmm, if this check were omitted, would this be insecure (or less secure)? 
Would
> we accidentally treat all nexes as having the same identity?  (Or would the
> identity be diluted?  We'd include fstat() metadata in the identity but not
the
> filename.)
> 
> How about changing NaClDescCreateWithFilePathMetadata() so that either:
> 
>  * it rejects an empty filename;
> 
>  * better: when given an empty filename, it falls back to the non-caching
> version (NaClDescIoDescFromDescAllocCtor() as below).  Then we wouldn't need 2
> code paths in this Chromium code.

Sounds good, I'll mail out a NaCl change that does the fallback in
NaClDescCreateWithFilePathMetadata shortly.

[teravest, 2014-10-16 17:07:43]

On 2014/10/14 18:15:01, teravest wrote:
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/browser...
> File components/nacl/browser/nacl_process_host.cc (right):
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/browser...
> components/nacl/browser/nacl_process_host.cc:910: void
> ClosePlatformFile(base::PlatformFile file) {
> On 2014/10/14 17:37:24, Mark Seaborn wrote:
> > Shouldn't this be in an anon namespace?  Doesn't base/ provide a function to
> > close a PlatformFile?
> 
> I've moved this to an anonymous namespace and changed to pass a base::File
> instead of having a routine to close a PlatformFile.
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
> File components/nacl/common/nacl_messages.h (right):
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
> components/nacl/common/nacl_messages.h:94:
> IPC_SYNC_MESSAGE_CONTROL2_2(NaClProcessMsg_ResolveFileToken,
> On 2014/10/14 17:37:24, Mark Seaborn wrote:
> > You can remove this, right?
> 
> Done.
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
> components/nacl/common/nacl_messages.h:100:
> IPC_MESSAGE_CONTROL2(NaClProcessMsg_ResolveFileTokenAsync,
> On 2014/10/14 17:37:24, Mark Seaborn wrote:
> > Should the "Async" suffix be removed from the name of this, since only the
> async
> > version remains?
> 
> Done.
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
> File components/nacl/common/nacl_types.h (right):
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/common/...
> components/nacl/common/nacl_types.h:66: base::FilePath nexe_file_path;
> On 2014/10/14 17:37:24, Mark Seaborn wrote:
> > Could you comment that this is used only as a key for validation caching? 
> > Otherwise it looks strange that a filename would be passed to a sandboxed
> > process that can't open files by name.
> > 
> > Ideally this would be named something more abstract like
> > "nexe_validation_cache_key", but I suppose the name should match up with the
> > NaClDescCreateWithFilePathMetadata() API.
> > 
> > So how about "nexe_file_path_metadata"?
> 
> Done.
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
> File components/nacl/loader/nacl_listener.cc (right):
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
> components/nacl/loader/nacl_listener.cc:36: #include
> "native_client/src/trusted/validator/rich_file_info.h"
> On 2014/10/14 17:37:24, Mark Seaborn wrote:
> > This isn't needed, is it?
> 
> Nope, removed.
> 
>
https://codereview.chromium.org/572973002/diff/200001/components/nacl/loader/...
> components/nacl/loader/nacl_listener.cc:437: if
(!params.nexe_file_path.empty())
> {
> On 2014/10/14 17:37:24, Mark Seaborn wrote:
> > Hmm, if this check were omitted, would this be insecure (or less secure)? 
> Would
> > we accidentally treat all nexes as having the same identity?  (Or would the
> > identity be diluted?  We'd include fstat() metadata in the identity but not
> the
> > filename.)
> > 
> > How about changing NaClDescCreateWithFilePathMetadata() so that either:
> > 
> >  * it rejects an empty filename;
> > 
> >  * better: when given an empty filename, it falls back to the non-caching
> > version (NaClDescIoDescFromDescAllocCtor() as below).  Then we wouldn't need
2
> > code paths in this Chromium code.
> 
> Sounds good, I'll mail out a NaCl change that does the fallback in
> NaClDescCreateWithFilePathMetadata shortly.

Done. The fallback now happens in NaClDescCreateWithFilePathMetadata.

[Mark Seaborn, 2014-10-16 18:01:01]

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (left):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:1022: // TODO(ncbray): track
behavior with UMA. If entries are getting evicted or
Why remove this comment?  AFAIK this still applies.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:917: base::File nexe_file) {
How about renaming this to "checked_nexe_file" or something similar?

Otherwise you have both "nexe_file" (safe) and "nexe_file_" (unsafe) in scope. 
It would be easy to make a mistake with that.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:919: // Release the file received
from the renderer. This has to be done on a
Does this mean that StartNaClExecution() and StartNaClFileResolved() are running
in different threads?  Wouldn't that mean that multiple threads are accessing
nexe_file_?  That doesn't sound like it would be thread-safe.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:34: #include
"native_client/src/trusted/desc/nacl_desc_io.h"
Not used now?

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:188: // This function is no longer used.
Can you add a TODO to remove ResolveFileToken() from
components/nacl/loader/nacl_validation_db.h?

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (left):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:147: CHECK(params.nexe_token_lo
== 0);
Shouldn't this check nexe_file_path_metadata is empty instead?

Leaking information about the user's filesystem would be a more serious leak
than leaking the tokens.  (On Chrome OS, these are probably generic filenames
that don't reveal anything, but I don't know that for sure.)

[teravest, 2014-10-16 20:57:44]

Patchset #4 (id:260001) has been deleted

[teravest, 2014-10-16 20:57:50]

Patchset #4 (id:280001) has been deleted

[teravest, 2014-10-16 21:40:40]

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (left):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:1022: // TODO(ncbray): track
behavior with UMA. If entries are getting evicted or
On 2014/10/16 18:01:00, Mark Seaborn wrote:
> Why remove this comment?  AFAIK this still applies.

Added this back.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:917: base::File nexe_file) {
On 2014/10/16 18:01:01, Mark Seaborn wrote:
> How about renaming this to "checked_nexe_file" or something similar?
> 
> Otherwise you have both "nexe_file" (safe) and "nexe_file_" (unsafe) in scope.

> It would be easy to make a mistake with that.

Done.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:919: // Release the file received
from the renderer. This has to be done on a
On 2014/10/16 18:01:01, Mark Seaborn wrote:
> Does this mean that StartNaClExecution() and StartNaClFileResolved() are
running
> in different threads?  Wouldn't that mean that multiple threads are accessing
> nexe_file_?  That doesn't sound like it would be thread-safe.

StartNaClExecution() and StartNaClFileResolved() are called on the same thread.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:34: #include
"native_client/src/trusted/desc/nacl_desc_io.h"
On 2014/10/16 18:01:01, Mark Seaborn wrote:
> Not used now?

Done.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:188: // This function is no longer used.
On 2014/10/16 18:01:01, Mark Seaborn wrote:
> Can you add a TODO to remove ResolveFileToken() from
> components/nacl/loader/nacl_validation_db.h?

I just removed ResolveFileToken there instead.

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (left):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:147: CHECK(params.nexe_token_lo
== 0);
On 2014/10/16 18:01:01, Mark Seaborn wrote:
> Shouldn't this check nexe_file_path_metadata is empty instead?
> 
> Leaking information about the user's filesystem would be a more serious leak
> than leaking the tokens.  (On Chrome OS, these are probably generic filenames
> that don't reveal anything, but I don't know that for sure.)

Done.

[Mark Seaborn, 2014-10-16 21:58:41]

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:919: // Release the file received
from the renderer. This has to be done on a
On 2014/10/16 21:40:40, teravest wrote:
> On 2014/10/16 18:01:01, Mark Seaborn wrote:
> > Does this mean that StartNaClExecution() and StartNaClFileResolved() are
> running
> > in different threads?  Wouldn't that mean that multiple threads are
accessing
> > nexe_file_?  That doesn't sound like it would be thread-safe.
> 
> StartNaClExecution() and StartNaClFileResolved() are called on the same
thread. 

Let me check I understand this:

StartNaClExecution() is called in the IO thread (in response to incoming IPCs).

StartNaClExecution() doesn't need to close a handle (so it doesn't need to
interact with the blocking-IO-thread).

Earlier we do:
PostTaskAndReplyWithResult(..., bind(OpenNaClReadExecImpl),
bind(StartNaClFileResolved))
-- This calls OpenNaClReadExecImpl on a blocking-file-IO thread, but it calls
StartNaClFileResolved on the IO thread.  (I hadn't realised about the latter
part before.)

https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:147:
CHECK(params.nexe_file_path_metadata.empty());
Have you tested this with packaged apps?  Packaged apps on Non-SFI mode aren't
tested on the bots, so I suspect this case will break, as it did in the case
that was fixed by https://codereview.chromium.org/560983003.

Could you add test coverage for that, please?

[teravest, 2014-10-20 18:20:25]

On Thu, Oct 16, 2014 at 3:58 PM,  <mseaborn@chromium.org> wrote:
>
>
https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
> File components/nacl/browser/nacl_process_host.cc (right):
>
>
https://codereview.chromium.org/572973002/diff/240001/components/nacl/browser...
> components/nacl/browser/nacl_process_host.cc:919: // Release the file
> received from the renderer. This has to be done on a
> On 2014/10/16 21:40:40, teravest wrote:
>>
>> On 2014/10/16 18:01:01, Mark Seaborn wrote:
>> > Does this mean that StartNaClExecution() and StartNaClFileResolved()
>
> are
>>
>> running
>> > in different threads?  Wouldn't that mean that multiple threads are
>
> accessing
>>
>> > nexe_file_?  That doesn't sound like it would be thread-safe.
>
>
>> StartNaClExecution() and StartNaClFileResolved() are called on the
>
> same thread.
>
> Let me check I understand this:
>
> StartNaClExecution() is called in the IO thread (in response to incoming
> IPCs).
>
> StartNaClExecution() doesn't need to close a handle (so it doesn't need
> to interact with the blocking-IO-thread).
>
> Earlier we do:
> PostTaskAndReplyWithResult(..., bind(OpenNaClReadExecImpl),
> bind(StartNaClFileResolved))
> -- This calls OpenNaClReadExecImpl on a blocking-file-IO thread, but it
> calls StartNaClFileResolved on the IO thread.  (I hadn't realised about
> the latter part before.)

That's correct.

>
>
https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
> File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):
>
>
https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_listener.cc:147:
> CHECK(params.nexe_file_path_metadata.empty());
> Have you tested this with packaged apps?  Packaged apps on Non-SFI mode
> aren't tested on the bots, so I suspect this case will break, as it did
> in the case that was fixed by https://codereview.chromium.org/560983003.
>
> Could you add test coverage for that, please?

I haven't tested this. You're probably right-- I do think I can fix
this pretty easily on the browser-side by not populating this field in
the StartParams if we're using Non-SFI mode.

dmichael sent me an old patch with code that had testing for a
packaged app. I'll take a look at that and try to add a test to get
coverage there.



>
> https://codereview.chromium.org/572973002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yusuke Sato, 2014-10-20 18:55:07]

yusukes@chromium.org changed reviewers:
+ yusukes@chromium.org

[Yusuke Sato, 2014-10-20 18:55:08]

https://codereview.chromium.org/572973002/diff/300001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/300001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:888: if
(NaClBrowser::GetInstance()->GetFilePath(nexe_token_.lo,
Is this necessary for non-sfi? Maybe

 if (!uses_nonsfi_mode_ && NaClBrowser...) {

?

[Yusuke Sato, 2014-10-21 21:51:53]

https://codereview.chromium.org/572973002/diff/300001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/300001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:888: if
(NaClBrowser::GetInstance()->GetFilePath(nexe_token_.lo,
and do you mean
 if (!NaClBrowser::GetInstance()->GetFilePath(...)) {
? It seems like NaClBrowser::GetFilePath() returns true on success.

[Yusuke Sato, 2014-10-22 00:24:29]

nevermind, I misread the code. Please disregard the comments.

[teravest, 2014-10-23 17:20:26]

This change shouldn't populate the file path field when nonsfi mode is used
anymore.

https://codereview.chromium.org/572973002/diff/300001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/572973002/diff/300001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:888: if
(NaClBrowser::GetInstance()->GetFilePath(nexe_token_.lo,
On 2014/10/20 18:55:08, Yusuke Sato wrote:
> Is this necessary for non-sfi? Maybe
> 
>  if (!uses_nonsfi_mode_ && NaClBrowser...) {
> 
> ?

Done.

[Mark Seaborn, 2014-11-13 16:36:28]

https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):

https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
components/nacl/loader/nonsfi/nonsfi_listener.cc:147:
CHECK(params.nexe_file_path_metadata.empty());
On 2014/10/16 21:58:40, Mark Seaborn wrote:
> Have you tested this with packaged apps?  Packaged apps on Non-SFI mode aren't
> tested on the bots, so I suspect this case will break, as it did in the case
> that was fixed by https://codereview.chromium.org/560983003.
> 
> Could you add test coverage for that, please?

It looks like you did that in https://codereview.chromium.org/723623002/
yesterday, so LGTM now, and thanks.  You could add a TEST= field that mentions
that test.

[teravest, 2014-11-13 16:39:28]

On 2014/11/13 16:36:28, Mark Seaborn wrote:
>
https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
> File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):
> 
>
https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
> components/nacl/loader/nonsfi/nonsfi_listener.cc:147:
> CHECK(params.nexe_file_path_metadata.empty());
> On 2014/10/16 21:58:40, Mark Seaborn wrote:
> > Have you tested this with packaged apps?  Packaged apps on Non-SFI mode
aren't
> > tested on the bots, so I suspect this case will break, as it did in the case
> > that was fixed by https://codereview.chromium.org/560983003.
> > 
> > Could you add test coverage for that, please?
> 
> It looks like you did that in https://codereview.chromium.org/723623002/
> yesterday, so LGTM now, and thanks.  You could add a TEST= field that mentions
> that test.

Sadly, https://codereview.chromium.org/723623002/ was recently reverted, as it's
failing on the buildbots, but not on the commit queue. Once I have that figured
out, I'll comment here with a link to the commit where the test was successfully
landed, and I'll add a test field.

Thanks for the LGTM.

[teravest, 2014-11-19 16:40:22]

On 2014/11/13 16:39:28, teravest wrote:
> On 2014/11/13 16:36:28, Mark Seaborn wrote:
> >
>
https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
> > File components/nacl/loader/nonsfi/nonsfi_listener.cc (right):
> > 
> >
>
https://codereview.chromium.org/572973002/diff/300001/components/nacl/loader/...
> > components/nacl/loader/nonsfi/nonsfi_listener.cc:147:
> > CHECK(params.nexe_file_path_metadata.empty());
> > On 2014/10/16 21:58:40, Mark Seaborn wrote:
> > > Have you tested this with packaged apps?  Packaged apps on Non-SFI mode
> aren't
> > > tested on the bots, so I suspect this case will break, as it did in the
case
> > > that was fixed by https://codereview.chromium.org/560983003.
> > > 
> > > Could you add test coverage for that, please?
> > 
> > It looks like you did that in https://codereview.chromium.org/723623002/
> > yesterday, so LGTM now, and thanks.  You could add a TEST= field that
mentions
> > that test.
> 
> Sadly, https://codereview.chromium.org/723623002/ was recently reverted, as
it's
> failing on the buildbots, but not on the commit queue. Once I have that
figured
> out, I'll comment here with a link to the commit where the test was
successfully
> landed, and I'll add a test field.
> 
> Thanks for the LGTM.

https://codereview.chromium.org/723623002/ was successfully re-landed, so there
is test coverage for packaged apps for Non-SFI mode now.

[teravest, 2014-11-19 17:34:53]

teravest@chromium.org changed reviewers:
+ tsepez@chromium.org

[teravest, 2014-11-19 17:34:53]

+tsepez for components/nacl/nacl_messages.h

This simplifies the codepath for validation caching for the main nexe for NaCl
plugins.

It removes a synchronous message that's no longer used.

It also replaces file tokens sent to the plugin with the file path information
(which was previously looked up from the tokens sent to the plugin).

[Tom Sepez, 2014-11-19 20:21:10]

Messages LGTM.

[teravest, 2014-11-19 20:25:17]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-11-19 20:26:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/572973002/340001

[commit-bot: I haz the power, 2014-11-20 03:55:37]

Committed patchset #6 (id:340001)

[commit-bot: I haz the power, 2014-11-20 03:56:11]

Patchset 6 (id:??) landed as
https://crrev.com/c6b3ad13c7dcf0b4359d75798463c2677f9c8e78
Cr-Commit-Position: refs/heads/master@{#304965}