Test open_resource IRT in NewlibPackagedAppTest.SuccessfulLoad

Test open_resource IRT in PackagedAppTest.SuccessfulLoad

The new test is similar to NaClBrowserTest*.IrtManifestFile, but unlike
the NaCl test, this one tests the "fast path" in DownloadNexe() in
ppb_nacl_private_impl.cc which opens resource files without using URLLoader.

TEST=trybot
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3802
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32

Committed: https://crrev.com/0b3c0ef146039e88136216f859daaf48d0c93e6f
Cr-Commit-Position: refs/heads/master@{#307424}

[Yusuke Sato, 2014-11-26 17:14:24]

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

[Yusuke Sato, 2014-11-26 17:14:40]

Justin, could you take a look at this?

[teravest, 2014-11-26 21:24:04]

lgtm

https://codereview.chromium.org/758383002/diff/20001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/20001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:52: while ((len =
read(desc, buffer, sizeof buffer - 1)) > 0) {
nit: I'd prefer "sizeof(buffer) - 1" instead of "sizeof buffer - 1"

https://codereview.chromium.org/758383002/diff/20001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:57: // Do not call
close(desc); so that LoadManifest calls the second
You don't need the semicolon here.

[Yusuke Sato, 2014-11-26 21:50:34]

thanks for the review!

https://codereview.chromium.org/758383002/diff/20001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/20001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:52: while ((len =
read(desc, buffer, sizeof buffer - 1)) > 0) {
On 2014/11/26 21:24:04, teravest wrote:
> nit: I'd prefer "sizeof(buffer) - 1" instead of "sizeof buffer - 1"

Done.

https://codereview.chromium.org/758383002/diff/20001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:57: // Do not call
close(desc); so that LoadManifest calls the second
On 2014/11/26 21:24:04, teravest wrote:
> You don't need the semicolon here.

Done.

[Yusuke Sato, 2014-11-26 21:50:48]

The CQ bit was checked by yusukes@chromium.org

[commit-bot: I haz the power, 2014-11-26 21:51:40]

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

[commit-bot: I haz the power, 2014-11-26 21:56:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-26 21:56:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yusuke Sato, 2014-11-26 21:58:17]

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

[Yusuke Sato, 2014-11-26 21:58:48]

Mark, could you review ppapi/tests/DEPS?

[Mark Seaborn, 2014-12-02 00:18:07]

Can you set BUG= to point to a public issue (such as
https://code.google.com/p/nativeclient/issues/detail?id=3802) rather than a
private one, please?

Is there a reason for putting this test in ppapi/tests/extensions/ rather than
adding it to chrome/test/data/nacl/manifest_file/?

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/DEPS
File ppapi/tests/DEPS (right):

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/DEPS#newcode23
ppapi/tests/DEPS:23: "+native_client/src/untrusted/nacl/nacl_irt.h",
See other comment about nacl_interface_query()...

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:25: //
ipc/file_descriptor_set_posix.h.
Why not #include that and use that constant here?  Also, that constant is
currrently set to 7.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:30: bool
LoadManifestInternal(TYPE_nacl_irt_query* query_func,
You can just call nacl_interface_query() here rather than passing a function
pointer around.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:57: // Do not call
close(desc) so that LoadManifest calls the second
Not sure I understand this comment.  Are you saying that calling close() could
cause the FD (and not the FD table index) to be reused?

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:95:
LoadManifest(&__nacl_irt_query);
This should use nacl_interface_query(), defined in <irt.h>, rather than
__nacl_irt_query, which is internal.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:110:
pthread_create(&thread, NULL, &RunTestsOnBackgroundThread, NULL);
Nit: check return value, if only with CHECK (or you could use thread-creation
functions from base/).

Can you add a comment to say why this is run on a background thread?

[Yusuke Sato, 2014-12-02 01:20:28]

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/DEPS
File ppapi/tests/DEPS (right):

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/DEPS#newcode23
ppapi/tests/DEPS:23: "+native_client/src/untrusted/nacl/nacl_irt.h",
On 2014/12/02 00:18:06, Mark Seaborn wrote:
> See other comment about nacl_interface_query()...

Removed.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:25: //
ipc/file_descriptor_set_posix.h.
On 2014/12/02 00:18:06, Mark Seaborn wrote:
> Why not #include that and use that constant here?

This is because Chrome's base/ now strongly depends on C++11 and doesn't seem to
compile with some of NaCl toolchains at all.

> Also, that constant is currrently set to 7.

Ok, changed to 7. I'll increase the number when I submit the other CL for
batch-opening resource files.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:30: bool
LoadManifestInternal(TYPE_nacl_irt_query* query_func,
On 2014/12/02 00:18:06, Mark Seaborn wrote:
> You can just call nacl_interface_query() here rather than passing a function
> pointer around.

Done.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:57: // Do not call
close(desc) so that LoadManifest calls the second
On 2014/12/02 00:18:06, Mark Seaborn wrote:
> Not sure I understand this comment.  Are you saying that calling close() could
> cause the FD (and not the FD table index) to be reused?

Removed the comment and added close(desc) instead. Since closed FDs will never
be reused, this test code was just confusing.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:95:
LoadManifest(&__nacl_irt_query);
On 2014/12/02 00:18:06, Mark Seaborn wrote:
> This should use nacl_interface_query(), defined in <irt.h>, rather than
> __nacl_irt_query, which is internal.

Done.

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:110:
pthread_create(&thread, NULL, &RunTestsOnBackgroundThread, NULL);
On 2014/12/02 00:18:06, Mark Seaborn wrote:
> Nit: check return value, if only with CHECK (or you could use thread-creation
> functions from base/).
> 
> Can you add a comment to say why this is run on a background thread?

Done.

[Yusuke Sato, 2014-12-02 01:21:18]

Mark: Please take another look.

[Yusuke Sato, 2014-12-02 02:04:38]

> Is there a reason for putting this test in ppapi/tests/extensions/ rather than
> adding it to chrome/test/data/nacl/manifest_file/?

Sorry, I missed this comment. I reused ppapi/tests/extensions/packaged_app/
since I wanted to test the same code as the existing
ppapi/tests/extensions/packaged_app/test_packaged_app.cc tests (but this time
via open_resource IRT).

If I understand correctly,
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc tests the slow
path (with URLLoader) of DownloadNexe() in ppb_nacl_private_impl.cc and this is
not what I wanted to test.

[Mark Seaborn, 2014-12-08 23:27:14]

On 2014/12/02 02:04:38, Yusuke Sato wrote:
> > Is there a reason for putting this test in ppapi/tests/extensions/ rather
than
> > adding it to chrome/test/data/nacl/manifest_file/?
> 
> Sorry, I missed this comment. I reused ppapi/tests/extensions/packaged_app/
> since I wanted to test the same code as the existing
> ppapi/tests/extensions/packaged_app/test_packaged_app.cc tests (but this time
> via open_resource IRT).

OK.  I think this is the same test case that Justin added/extended in
https://codereview.chromium.org/723623002/.

LGTM

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:25: //
ipc/file_descriptor_set_posix.h.
On 2014/12/02 01:20:28, Yusuke Sato wrote:
> On 2014/12/02 00:18:06, Mark Seaborn wrote:
> > Why not #include that and use that constant here?
> 
> This is because Chrome's base/ now strongly depends on C++11 and doesn't seem
to
> compile with some of NaCl toolchains at all.

Oh, I see.  That isn't obvious at all, so can you add a comment and a TODO to
fix that and use FileDescriptorSet::kMaxDescriptorsPerMessage?

How much of base/ fails to compile for NaCl tests?  We knew this would be likely
to happen, and that we'd have to decide which tests we continue to build against
the old nacl-gcc toolchains.

+dschuff (who is working on the nacl-clang toolchain).  Derek, could you
investigate the test situation and maybe file an issue about this?

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:49: close(desc);
Nit: Check return value

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:77: std::string content
= "Test File Content" + std::string(i % 2 ? "2" : "");
BTW, something like "Example contents for open_resource test" would be more
greppable (less likely to be the same in another test)

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:91:
g_instance->PostMessage("hello");
Maybe change "hello" to "Test passed" now that the alternative case is an error
message.

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:116:
pthread_detach(thread);
Nit: check return value (e.g. with CHECK)

[Yusuke Sato, 2014-12-09 03:45:55]

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:25: //
ipc/file_descriptor_set_posix.h.
On 2014/12/08 23:27:14, Mark Seaborn wrote:
> On 2014/12/02 01:20:28, Yusuke Sato wrote:
> > On 2014/12/02 00:18:06, Mark Seaborn wrote:
> > > Why not #include that and use that constant here?
> > 
> > This is because Chrome's base/ now strongly depends on C++11 and doesn't
seem
> to
> > compile with some of NaCl toolchains at all.
> 
> Oh, I see.  That isn't obvious at all, so can you add a comment and a TODO to
> fix that and use FileDescriptorSet::kMaxDescriptorsPerMessage?

Added a TODO. Done.

> How much of base/ fails to compile for NaCl tests?  We knew this would be
likely
> to happen, and that we'd have to decide which tests we continue to build
against
> the old nacl-gcc toolchains.
> 
> +dschuff (who is working on the nacl-clang toolchain).  Derek, could you
> investigate the test situation and maybe file an issue about this?

Since I only tried to include "ipc/file_descriptor_set_posix.h", I'm not sure
how much of base/ is incompatible with NaCl toolchains unfortunately. As for
ipc/file_descriptor_set_posix.h, it's incompatible because some of the
toolchains don't support 'override' and 'nullptr'.

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:49: close(desc);
On 2014/12/08 23:27:14, Mark Seaborn wrote:
> Nit: Check return value

Done.

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:77: std::string content
= "Test File Content" + std::string(i % 2 ? "2" : "");
On 2014/12/08 23:27:14, Mark Seaborn wrote:
> BTW, something like "Example contents for open_resource test" would be more
> greppable (less likely to be the same in another test)

Done.

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:91:
g_instance->PostMessage("hello");
On 2014/12/08 23:27:14, Mark Seaborn wrote:
> Maybe change "hello" to "Test passed" now that the alternative case is an
error
> message.

Done.

https://codereview.chromium.org/758383002/diff/60001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:116:
pthread_detach(thread);
On 2014/12/08 23:27:14, Mark Seaborn wrote:
> Nit: check return value (e.g. with CHECK)

Done.

[Yusuke Sato, 2014-12-09 03:46:05]

The CQ bit was checked by yusukes@chromium.org

[commit-bot: I haz the power, 2014-12-09 03:47:05]

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

[commit-bot: I haz the power, 2014-12-09 05:55:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-09 05:55:43]

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