Set file tokens for NaCl main nexe if available to enable validation caching.

Set file tokens for NaCl main nexe if available to enable validation caching.

Also enable mmap'ing the main nexe, which may help a bit
in subsequent runs as well (first run the validator needs
page in all of the text anyway).

pnacl-llc.nexe, etc will be handled separately:
https://codereview.chromium.org/224803002/

TEST=NaClBrowserTestVcacheExtension.ValidationCacheOfMainNexe

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3421

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

[teravest, 2014-04-30 20:12:54]

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:498: if (tmp_info.file_token.lo
!= 0 && tmp_info.file_token.hi != 0) {
Should this be ||, not &&? Is it possible that lo or hi could be 0 for valid
token data?

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:499: if
(!NaClDescSetFileToken(wrapper->desc(), &tmp_info.file_token)) {
It's a shame that there's not a call to MakeFileDesc() that can take a
NaClFileInfo instead.

[jvoung (off chromium), 2014-04-30 21:49:02]

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:498: if (tmp_info.file_token.lo
!= 0 && tmp_info.file_token.hi != 0) {
On 2014/04/30 20:12:54, teravest wrote:
> Should this be ||, not &&? Is it possible that lo or hi could be 0 for valid
> token data?

Yeah I think having only one of them be 0 is still accepted.  I guess the
concern is if some code accidentally left one of the hi/lo at 0 all the time
(instead of just picking 0 by chance):

https://chromium.googlesource.com/native_client/src/native_client/+/refs/head...

And see NaClBrowser::PutFilePath which marches forth once of the tokens is
randomly chosen to be non-zero.

https://chromium.googlesource.com/chromium/chromium/+/trunk/components/nacl/b...

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:499: if
(!NaClDescSetFileToken(wrapper->desc(), &tmp_info.file_token)) {
On 2014/04/30 20:12:54, teravest wrote:
> It's a shame that there's not a call to MakeFileDesc() that can take a
> NaClFileInfo instead.

Okay, maybe we can add that:
https://codereview.chromium.org/261683002/

Thoughts, Nick?

[teravest, 2014-05-02 15:07:24]

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:498: if (tmp_info.file_token.lo
!= 0 && tmp_info.file_token.hi != 0) {
On 2014/04/30 21:49:02, jvoung (wakacje 5th to 13th) wrote:
> On 2014/04/30 20:12:54, teravest wrote:
> > Should this be ||, not &&? Is it possible that lo or hi could be 0 for valid
> > token data?
> 
> Yeah I think having only one of them be 0 is still accepted.  I guess the
> concern is if some code accidentally left one of the hi/lo at 0 all the time
> (instead of just picking 0 by chance):
> 
>
https://chromium.googlesource.com/native_client/src/native_client/+/refs/head...
> 
> And see NaClBrowser::PutFilePath which marches forth once of the tokens is
> randomly chosen to be non-zero.
> 
>
https://chromium.googlesource.com/chromium/chromium/+/trunk/components/nacl/b...

I'm confused. If you want to use && here, you should probably change
NaClBrowser::PutFilePath to match.

[jvoung (off chromium), 2014-05-02 15:33:23]

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:498: if (tmp_info.file_token.lo
!= 0 && tmp_info.file_token.hi != 0) {
On 2014/05/02 15:07:25, teravest wrote:
> On 2014/04/30 21:49:02, jvoung (wakacje 5th to 13th) wrote:
> > On 2014/04/30 20:12:54, teravest wrote:
> > > Should this be ||, not &&? Is it possible that lo or hi could be 0 for
valid
> > > token data?
> > 
> > Yeah I think having only one of them be 0 is still accepted.  I guess the
> > concern is if some code accidentally left one of the hi/lo at 0 all the time
> > (instead of just picking 0 by chance):
> > 
> >
>
https://chromium.googlesource.com/native_client/src/native_client/+/refs/head...
> > 
> > And see NaClBrowser::PutFilePath which marches forth once of the tokens is
> > randomly chosen to be non-zero.
> > 
> >
>
https://chromium.googlesource.com/chromium/chromium/+/trunk/components/nacl/b...
> 
> I'm confused. If you want to use && here, you should probably change
> NaClBrowser::PutFilePath to match.

Oops sorry, messed up the negation logic. Changed to || to match.

[Nick Bray (chromium), 2014-05-02 18:18:19]

LGTM

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:498: if (tmp_info.file_token.lo
!= 0 && tmp_info.file_token.hi != 0) {
Justin is correct, although I'd go further and suggest this for clarity:
!(file_token->lo == 0 && file_token->hi == 0) .

See:
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...

Maybe this should even be factored into its own function, given a name, and
shared?  Unclear if that would be a win.

[jvoung (off chromium), 2014-05-02 19:26:15]

Thanks Nick -- and I keep forgetting that your OWNERS entry under NaCl is w/
your @google.com address (DEPS file modifications seem to ask for OWNERS)

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:498: if (tmp_info.file_token.lo
!= 0 && tmp_info.file_token.hi != 0) {
On 2014/05/02 18:18:19, Nick Bray (chromium) wrote:
> Justin is correct, although I'd go further and suggest this for clarity:
> !(file_token->lo == 0 && file_token->hi == 0) .
> 
> See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...
> 
> Maybe this should even be factored into its own function, given a name, and
> shared?  Unclear if that would be a win.


Yeah, I could put something in nacl_file_info.h, so that what's considered valid
is not just a comment in that file but a function that could be reused.

Then I can clean this up after a NaCl -> Chrome roll.

This particular check could also be folded into the desc_wrapper factory method
proposed in the other CL if that makes sense.

[teravest, 2014-05-07 14:15:22]

lgtm

[jvoung (off chromium), 2014-05-20 16:12:41]

Thanks for the earlier review -- I ended up rebasing and adding a test using UMA
stats (so no need to add a test syscall which was the earlier plan)

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:499: if
(!NaClDescSetFileToken(wrapper->desc(), &tmp_info.file_token)) {
Changed to use the new ctor function that passes both the desc and file token.

[teravest, 2014-05-21 16:49:42]

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
File
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js
(right):

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js:1:
function create(manifest_url) {
Do you need a copyright statement for this file?

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:78: 'build_glibc': 0,
Are you only building newlib just to make testing faster?

https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:452: nexe_file_info_.desc =
ConvertFileDescriptor(nexe_handle_);
I'm a little confused why nexe_file_info_ needs to be a class member; it might
be simpler to just create the NaClFileInfo inline in this function, and have
Plugin class members for token_lo and token_hi.

I have a similar change for dealing with files downloaded for open_resource():
https://codereview.chromium.org/294633003/

...and ended up making a PP_NaClFileInfo type that I passed around, which seemed
to work alright.

[jvoung (off chromium), 2014-05-21 21:10:56]

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
File
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js
(right):

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js:1:
function create(manifest_url) {
On 2014/05/21 16:49:42, teravest wrote:
> Do you need a copyright statement for this file?

Done.

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
File chrome/test/data/nacl/nacl_test_data.gyp (right):

https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
chrome/test/data/nacl/nacl_test_data.gyp:78: 'build_glibc': 0,
On 2014/05/21 16:49:42, teravest wrote:
> Are you only building newlib just to make testing faster?

No it's not just the testing time. There are some assumptions in the test that
are specific to newlib right now:

- the number of validation queries/settings varies.  It would be more with glibc
-- from the open_resource uses, and fewer for PNaCl because the main nexe is
placed in a temp file.
- the tester jig is hardcoded to inherit from the newlib class
- the test directory path commandline flag for loading the unpacked extension is
specific to newlib right now too

It might be good to at least add the glibc case but I just didn't do that yet.

Added a comment if that helps.

https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:452: nexe_file_info_.desc =
ConvertFileDescriptor(nexe_handle_);
On 2014/05/21 16:49:42, teravest wrote:
> I'm a little confused why nexe_file_info_ needs to be a class member; it might
> be simpler to just create the NaClFileInfo inline in this function, and have
> Plugin class members for token_lo and token_hi.

It was mostly to avoid heap allocating the NaClFileInfo, but yeah I can make the
members token_lo and token_hi instead and only reconstitute the NaClFileInfo
here.

> 
> I have a similar change for dealing with files downloaded for open_resource():
> https://codereview.chromium.org/294633003/
> 
> ...and ended up making a PP_NaClFileInfo type that I passed around, which
seemed
> to work alright.

Should I leave the ppb_nacl interface having separate desc/tokens for now, and
switch to PP_NaClFileInfo later?

[chromium-reviews, 2014-05-21 21:16:16]

On Wed, May 21, 2014 at 3:10 PM,  <jvoung@chromium.org> wrote:
>
>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
> File
>
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js
> (right):
>
>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
>
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js:1:
> function create(manifest_url) {
> On 2014/05/21 16:49:42, teravest wrote:
>>
>> Do you need a copyright statement for this file?
>
>
> Done.
>
>
>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
> File chrome/test/data/nacl/nacl_test_data.gyp (right):
>
>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
> chrome/test/data/nacl/nacl_test_data.gyp:78: 'build_glibc': 0,
> On 2014/05/21 16:49:42, teravest wrote:
>>
>> Are you only building newlib just to make testing faster?
>
>
> No it's not just the testing time. There are some assumptions in the
> test that are specific to newlib right now:
>
> - the number of validation queries/settings varies.  It would be more
> with glibc -- from the open_resource uses, and fewer for PNaCl because
> the main nexe is placed in a temp file.
> - the tester jig is hardcoded to inherit from the newlib class
> - the test directory path commandline flag for loading the unpacked
> extension is specific to newlib right now too
>
> It might be good to at least add the glibc case but I just didn't do
> that yet.
>
> Added a comment if that helps.

Yes, the comment is helpful. Thanks for adding it!

>
>
>
https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
> File ppapi/native_client/src/trusted/plugin/plugin.cc (right):
>
>
https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
> ppapi/native_client/src/trusted/plugin/plugin.cc:452:
> nexe_file_info_.desc = ConvertFileDescriptor(nexe_handle_);
> On 2014/05/21 16:49:42, teravest wrote:
>>
>> I'm a little confused why nexe_file_info_ needs to be a class member;
>
> it might
>>
>> be simpler to just create the NaClFileInfo inline in this function,
>
> and have
>>
>> Plugin class members for token_lo and token_hi.
>
>
> It was mostly to avoid heap allocating the NaClFileInfo, but yeah I can
> make the members token_lo and token_hi instead and only reconstitute the
> NaClFileInfo here.
>
>
>
>> I have a similar change for dealing with files downloaded for
>
> open_resource():
>>
>> https://codereview.chromium.org/294633003/
>
>
>> ...and ended up making a PP_NaClFileInfo type that I passed around,
>
> which seemed
>>
>> to work alright.
>
>
> Should I leave the ppb_nacl interface having separate desc/tokens for
> now, and switch to PP_NaClFileInfo later?

Whatever's most convenient for you. I just didn't want the plugin to
effectively have two members for the nexe handle (the nexe_handle_,
and the desc inside NaClFileInfo). What you've got now looks much
better.

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

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

[chromium-reviews, 2014-05-21 21:16:40]

lgtm

On Wed, May 21, 2014 at 3:15 PM, Justin TerAvest <teravest@google.com> wrote:
> On Wed, May 21, 2014 at 3:10 PM,  <jvoung@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
>> File
>>
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js
>> (right):
>>
>>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/e...
>>
chrome/test/data/nacl/extension_validation_cache/extension_validation_cache.js:1:
>> function create(manifest_url) {
>> On 2014/05/21 16:49:42, teravest wrote:
>>>
>>> Do you need a copyright statement for this file?
>>
>>
>> Done.
>>
>>
>>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
>> File chrome/test/data/nacl/nacl_test_data.gyp (right):
>>
>>
https://codereview.chromium.org/263683002/diff/220001/chrome/test/data/nacl/n...
>> chrome/test/data/nacl/nacl_test_data.gyp:78: 'build_glibc': 0,
>> On 2014/05/21 16:49:42, teravest wrote:
>>>
>>> Are you only building newlib just to make testing faster?
>>
>>
>> No it's not just the testing time. There are some assumptions in the
>> test that are specific to newlib right now:
>>
>> - the number of validation queries/settings varies.  It would be more
>> with glibc -- from the open_resource uses, and fewer for PNaCl because
>> the main nexe is placed in a temp file.
>> - the tester jig is hardcoded to inherit from the newlib class
>> - the test directory path commandline flag for loading the unpacked
>> extension is specific to newlib right now too
>>
>> It might be good to at least add the glibc case but I just didn't do
>> that yet.
>>
>> Added a comment if that helps.
>
> Yes, the comment is helpful. Thanks for adding it!
>
>>
>>
>>
https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
>> File ppapi/native_client/src/trusted/plugin/plugin.cc (right):
>>
>>
https://codereview.chromium.org/263683002/diff/220001/ppapi/native_client/src...
>> ppapi/native_client/src/trusted/plugin/plugin.cc:452:
>> nexe_file_info_.desc = ConvertFileDescriptor(nexe_handle_);
>> On 2014/05/21 16:49:42, teravest wrote:
>>>
>>> I'm a little confused why nexe_file_info_ needs to be a class member;
>>
>> it might
>>>
>>> be simpler to just create the NaClFileInfo inline in this function,
>>
>> and have
>>>
>>> Plugin class members for token_lo and token_hi.
>>
>>
>> It was mostly to avoid heap allocating the NaClFileInfo, but yeah I can
>> make the members token_lo and token_hi instead and only reconstitute the
>> NaClFileInfo here.
>>
>>
>>
>>> I have a similar change for dealing with files downloaded for
>>
>> open_resource():
>>>
>>> https://codereview.chromium.org/294633003/
>>
>>
>>> ...and ended up making a PP_NaClFileInfo type that I passed around,
>>
>> which seemed
>>>
>>> to work alright.
>>
>>
>> Should I leave the ppb_nacl interface having separate desc/tokens for
>> now, and switch to PP_NaClFileInfo later?
>
> Whatever's most convenient for you. I just didn't want the plugin to
> effectively have two members for the nexe handle (the nexe_handle_,
> and the desc inside NaClFileInfo). What you've got now looks much
> better.
>
>>
>> https://codereview.chromium.org/263683002/

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

[jvoung (off chromium), 2014-05-21 22:15:48]

Thanks!

+sky for chrome/test/base/uma_histogram_helper OWNERS

[sky, 2014-05-21 22:59:08]

LGTM

[jvoung (off chromium), 2014-05-22 17:32:14]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2014-05-22 17:34:40]

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

[commit-bot: I haz the power, 2014-05-22 20:55:12]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[jvoung (off chromium), 2014-05-22 20:56:04]

The CQ bit was checked by jvoung@chromium.org

[commit-bot: I haz the power, 2014-05-22 20:57:50]

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

[commit-bot: I haz the power, 2014-05-23 07:26:10]

Change committed as 272429