Allow to have different files for different ISAs in NaCl manifest.

Allow having different files for different ISAs in NaCl manifest.

Currently all files must have version for current ISA or
NaCl program will not load.  I relaxed this condition.  This 
is useful when ARM version of the program is compiled with
newlib but x86 version is compiled with glibc.  Developer
doesn't need to add ARM versions for all *.so libraries in
this case.

Since not having current architecture for a file is no 
longer an error, I changed ppapi_bad_manifest_bad_files.nmf
to test for a different error in file section.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3049
TEST= run_manifest_browser_test and run_ppapi_bad_browser_test

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

[halyavin2, 2013-04-10 14:08:09]

I don't know why but Bennet is not in OWNERS for manifest parser.

[Mark Seaborn, 2013-04-10 14:46:57]

s/Allow to have/Allow having/
s/usefull/useful/

Could you add a test for this?

[halyavin2, 2013-04-10 15:23:47]

On 2013/04/10 14:46:57, Mark Seaborn wrote:
> s/Allow to have/Allow having/
> s/usefull/useful/
> 
> Could you add a test for this?

I found that ppapi_bad.html allows to write the test I need.  I will look into
details tomorrow.

[halyavin2, 2013-04-11 10:33:19]

I added one test. PTAL.

[halyavin, 2013-04-12 11:40:21]

ping.


2013/4/11 <halyavin@chromium.org>

> I added one test. PTAL.
>
>
https://codereview.chromium.**org/14069002/<https://codereview.chromium.org/1...
>

[Mark Seaborn, 2013-04-12 22:56:03]

On 2013/04/12 11:40:21, halyavin wrote:
> ping.

I didn't get to this today, sorry -- it was a busy day and I'm not very familiar
with the manifest-loading code.  I'll look next week.

[Mark Seaborn, 2013-04-18 01:17:46]

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:255: if
(!sandbox_isa.empty()) {
Why not remove this whole block?

AFAICS, this block would only apply to two cases:

 * The "program" section.  But the NaCl app will fail to start anyway if there's
no program specified for the current arch!

 * The "interpreter" section.  But this section is unused, AFAIK.  We should
probably remove the code that looks for it.

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:106: //
'bad_manifest_bad_files' loads a manifest with a bad 'files' section.
Isn't the purpose of this test to test for the requirement that you're removing?
 Could you explain more about how you're updating the tests?

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:111: 'NaCl module
load failed: manifiest: file.txt property \'unknown_arch\' does not have
required key: \'pnacl-translate\'.');
"manifiest" -> "manifest".  This seems to be a typo in the source before your
change.  Would you mind fixing it?

I'm not sure why you should expect to get an error about "pnacl-translate" here?

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/manifest/manifest_arch_specific.nmf
(right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/manifest/manifest_arch_specific.nmf:11:
"x86-32": { "url": "subdir/manifest_good_x86-32.nexe"}
Remove space before '"url"'.  Same below.

[halyavin, 2013-04-18 07:33:26]

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/json_manifest.cc (right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/json_manifest.cc:255: if
(!sandbox_isa.empty()) {
On 2013/04/18 01:17:46, Mark Seaborn wrote:
> Why not remove this whole block?
> 
> AFAICS, this block would only apply to two cases:
> 
>  * The "program" section.  But the NaCl app will fail to start anyway if
there's
> no program specified for the current arch!
Once we remove the whole block, absence of URL will be treated as an empty URL. 
Empty relative URL is valid, so we may have some weird error messages instead. 
There is probably a better way to implement this part of manifest parsing but I
don't want to make major changes now.

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:106: //
'bad_manifest_bad_files' loads a manifest with a bad 'files' section.
On 2013/04/18 01:17:46, Mark Seaborn wrote:
> Isn't the purpose of this test to test for the requirement that you're
removing?
Not quite. It is tests for bad 'files' section. Since not having current arch is
no more an error, I used a different error in 'files' section for this test.

In order to test a positive case, I added one more 'good' test. It has a file
that is not available for any architectures.

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:111: 'NaCl module
load failed: manifiest: file.txt property \'unknown_arch\' does not have
required key: \'pnacl-translate\'.');
On 2013/04/18 01:17:46, Mark Seaborn wrote:
> "manifiest" -> "manifest".  This seems to be a typo in the source before your
> change.  Would you mind fixing it?
Done.
> 
> I'm not sure why you should expect to get an error about "pnacl-translate"
here?
The code looks for 2 keys: 'url' and 'pnacl-translate'. If neither is found it
complains about the last one.

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/manifest/manifest_arch_specific.nmf
(right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/manifest/manifest_arch_specific.nmf:11:
"x86-32": { "url": "subdir/manifest_good_x86-32.nexe"}
On 2013/04/18 01:17:46, Mark Seaborn wrote:
> Remove space before '"url"'.  Same below.

Done.

[Mark Seaborn, 2013-04-19 05:36:43]

LGTM

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:106: //
'bad_manifest_bad_files' loads a manifest with a bad 'files' section.
On 2013/04/18 07:33:27, halyavin wrote:
> On 2013/04/18 01:17:46, Mark Seaborn wrote:
> > Isn't the purpose of this test to test for the requirement that you're
> removing?
> Not quite. It is tests for bad 'files' section. Since not having current arch
is
> no more an error, I used a different error in 'files' section for this test.

OK, please add a note to the commit message explaining that.

https://codereview.chromium.org/14069002/diff/40001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/14069002/diff/40001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:111: 'NaCl module
load failed: manifest: file.txt property \'unknown_arch\' does not have required
key: \'pnacl-translate\'.');
Please add a comment here explaining the test expectation:  "The code looks for
2 keys: 'url' and 'pnacl-translate'. If neither is found it complains about the
last one" -- as you said.

[halyavin, 2013-04-19 06:41:51]

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/14069002/diff/23001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:106: //
'bad_manifest_bad_files' loads a manifest with a bad 'files' section.
On 2013/04/19 05:36:43, Mark Seaborn wrote:
> On 2013/04/18 07:33:27, halyavin wrote:
> > On 2013/04/18 01:17:46, Mark Seaborn wrote:
> > > Isn't the purpose of this test to test for the requirement that you're
> > removing?
> > Not quite. It is tests for bad 'files' section. Since not having current
arch
> is
> > no more an error, I used a different error in 'files' section for this test.
> 
> OK, please add a note to the commit message explaining that.

Done.

https://codereview.chromium.org/14069002/diff/40001/ppapi/native_client/tests...
File ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html (right):

https://codereview.chromium.org/14069002/diff/40001/ppapi/native_client/tests...
ppapi/native_client/tests/ppapi_browser/bad/ppapi_bad.html:111: 'NaCl module
load failed: manifest: file.txt property \'unknown_arch\' does not have required
key: \'pnacl-translate\'.');
On 2013/04/19 05:36:43, Mark Seaborn wrote:
> Please add a comment here explaining the test expectation:  "The code looks
for
> 2 keys: 'url' and 'pnacl-translate'. If neither is found it complains about
the
> last one" -- as you said.

Done.

[commit-bot: I haz the power, 2013-04-19 06:42:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/14069002/50001

[commit-bot: I haz the power, 2013-04-19 12:57:07]

Change committed as 195145