NaCl: Remove SRPC-based manifest tests.

NaCl: Remove SRPC-based manifest tests.

irt_open_resource() is being refactored to stop using SRPC.
Testing should happen via the IRT instead of using the SRPC interfaces directly.

This change removes these tests entirely since an upcoming change will make
this SRPC interface unusable, except by PNaCl translator processes:
  https://codereview.chromium.org/418423002/

BUG=394130
R=mseaborn@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/0dba8de00d52b6b1e6cb12a09f1f99281d6f40b5

[teravest, 2014-09-03 16:34:47]

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

[Mark Seaborn, 2014-09-03 17:06:39]

LGTM, but you might want to check test coverage.

https://codereview.chromium.org/535223002/diff/1/chrome/test/data/nacl/manife...
File chrome/test/data/nacl/manifest_file/pm_pre_init_manifest_file_test.cc
(left):

https://codereview.chromium.org/535223002/diff/1/chrome/test/data/nacl/manife...
chrome/test/data/nacl/manifest_file/pm_pre_init_manifest_file_test.cc:239:
return PpapiPluginMain();
It looks like this is testing that open_resource() works before PPAPI is
initialised (via ppapi_start()).  Do we have any targeted tests for this case? 
We would be testing it via the nacl-glibc tests (using dynamic linking), but
that's not targeted testing.

Hmm, it looks like chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc
does test open_resource() before ppapi_start(), since it does this:

int main() {
  result.push_back(LoadManifestSuccess(&__nacl_irt_query));
  result.push_back(LoadManifestNonExistentEntry(&__nacl_irt_query));
  result.push_back(LoadManifestNonExistentFile(&__nacl_irt_query));
  return PpapiPluginMain();
}

So then the question is, do we have any targeted tests checking that
open_resource() works *after* PPAPI is initialised?  Looks like that would be
easy to add to irt_manifest_file_test.cc by running those functions a second
time after PpapiPluginMain().

[teravest, 2014-09-03 19:39:00]

Patchset #2 (id:20001) has been deleted

[teravest, 2014-09-03 21:04:51]

I've added test coverage for calling open_resource() after PPAPI starts.

This can't be called after PpapiPluginMain() since the main loop for Pepper will
have exited at that point, so messages can't be posted back to JavaScript.
Additionally, open_resource() can't be called on the main thread after ppapi
starts (presumably so the main thread isn't blocked). So I did some shuffling to
call open_resource() on a background thread.

[teravest, 2014-09-04 15:56:51]

Committed patchset #3 (id:60001) manually as 0dba8de (presubmit successful).

[Mark Seaborn, 2014-09-04 17:41:36]

LGTM

BTW, with my earlier "LGTM" I meant 'you should commit this as-is and consider
addressing the test coverage in a separate change'.  Otherwise if you change the
CL in a non-trivial way, I'd expect to re-review it.  Not a big deal though!

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (right):

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:133:
pp::Module::Get()->core()->CallOnMainThread(
BTW, why does this require a CallOnMainThread()?  I thought
g_instance->PostMessage() would work on any thread.

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:158:
pthread_create(&thread_, NULL, &RunTestsOnBackgroundThread, NULL);
Nit: can you check the return value?

Nit: thread_ could be a local var instead -- doesn't need to be a member.

[teravest, 2014-09-04 20:21:28]

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (right):

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:133:
pp::Module::Get()->core()->CallOnMainThread(
On 2014/09/04 17:41:36, Mark Seaborn wrote:
> BTW, why does this require a CallOnMainThread()?  I thought
> g_instance->PostMessage() would work on any thread.

pp::Instance::PostMessage() has to be called from the main thread. I could call
this from a background thread if I used PPB_Instance, it sounds like.

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:158:
pthread_create(&thread_, NULL, &RunTestsOnBackgroundThread, NULL);
On 2014/09/04 17:41:36, Mark Seaborn wrote:
> Nit: can you check the return value?
> 
> Nit: thread_ could be a local var instead -- doesn't need to be a member.

Sure, do you want me to write a change to fix up these nits?

[Mark Seaborn, 2014-09-04 22:00:41]

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (right):

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:133:
pp::Module::Get()->core()->CallOnMainThread(
On 2014/09/04 20:21:28, teravest wrote:
> On 2014/09/04 17:41:36, Mark Seaborn wrote:
> > BTW, why does this require a CallOnMainThread()?  I thought
> > g_instance->PostMessage() would work on any thread.
> 
> pp::Instance::PostMessage() has to be called from the main thread. I could
call
> this from a background thread if I used PPB_Instance, it sounds like.

Hmm, you mean the C++ wrappers for PPAPI add a thread restriction?

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:158:
pthread_create(&thread_, NULL, &RunTestsOnBackgroundThread, NULL);
On 2014/09/04 20:21:28, teravest wrote:
> On 2014/09/04 17:41:36, Mark Seaborn wrote:
> > Nit: can you check the return value?
> > 
> > Nit: thread_ could be a local var instead -- doesn't need to be a member.
> 
> Sure, do you want me to write a change to fix up these nits?

It's your call -- I'll leave it up to you.

[teravest, 2014-09-04 22:12:40]

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (right):

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:133:
pp::Module::Get()->core()->CallOnMainThread(
On 2014/09/04 22:00:41, Mark Seaborn wrote:
> On 2014/09/04 20:21:28, teravest wrote:
> > On 2014/09/04 17:41:36, Mark Seaborn wrote:
> > > BTW, why does this require a CallOnMainThread()?  I thought
> > > g_instance->PostMessage() would work on any thread.
> > 
> > pp::Instance::PostMessage() has to be called from the main thread. I could
> call
> > this from a background thread if I used PPB_Instance, it sounds like.
> 
> Hmm, you mean the C++ wrappers for PPAPI add a thread restriction?

I believe that only happens for pp::Instance.
There's an explanatory comment here:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/instance...

And the pp::Instance object is deleted here:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/module.c...

Thanks to dmichael@ for pointing me at the details of this one.

[dmichael (off chromium), 2014-09-05 19:44:08]

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

[dmichael (off chromium), 2014-09-05 19:44:08]

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
File chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc (right):

https://codereview.chromium.org/535223002/diff/60001/chrome/test/data/nacl/ma...
chrome/test/data/nacl/manifest_file/irt_manifest_file_test.cc:133:
pp::Module::Get()->core()->CallOnMainThread(
On 2014/09/04 22:12:39, teravest wrote:
> On 2014/09/04 22:00:41, Mark Seaborn wrote:
> > On 2014/09/04 20:21:28, teravest wrote:
> > > On 2014/09/04 17:41:36, Mark Seaborn wrote:
> > > > BTW, why does this require a CallOnMainThread()?  I thought
> > > > g_instance->PostMessage() would work on any thread.
> > > 
> > > pp::Instance::PostMessage() has to be called from the main thread. I could
> > call
> > > this from a background thread if I used PPB_Instance, it sounds like.
> > 
> > Hmm, you mean the C++ wrappers for PPAPI add a thread restriction?
> 
> I believe that only happens for pp::Instance.
> There's an explanatory comment here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/instance...
> 
> And the pp::Instance object is deleted here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/module.c...
> 
> Thanks to dmichael@ for pointing me at the details of this one.

In theory it doesn't matter for NaCl, since we just kill the process without
calling DidDestroy. But it's safer to not make assumptions about pp::Instance's
lifetime from other threads. pp::Instance's API doesn't really make it possible
for it to be thread-safe, unfortunately...  but it was designed before we had
thread-safe Pepper calls.

Most of the C++ wrappers work fine because the object is really just a handle
that holds a PP_Resource (and addrefs and removerefs the resource at appropriate
times). If the same PP_Resource is used from multiple threads, the best thing
would be for each thread to have its own pp::Foo (since it's just a handle
anyway).

[commit-bot: I haz the power, 2014-09-10 03:31:35]

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