Init Mojo embedder for Windows NaCl plugins.

Init Mojo embedder for Windows NaCl plugins.

With this change (and another fix for NACL_BUILD_SUBARCH), we can test
Pepper/Mojo/NaCl support on Windows.

This also moves embedder initialization to nacl_helper for clarity:
The Mojo embedder must be initialized in nacl_helper when that process
is used. When the plugin is running in a chrome process, ContentMain()
handles Mojo embedder initialization.

BUG=414804
R=dmichael@chromium.org, ncbray@chromium.org

Committed: https://crrev.com/75386dca6ebff785e62eb52472e660c757d5b2e8
Cr-Commit-Position: refs/heads/master@{#323750}

[teravest, 2015-03-31 20:01:26]

Patchset #5 (id:80001) has been deleted

[teravest, 2015-03-31 20:01:32]

Patchset #3 (id:40001) has been deleted

[teravest, 2015-03-31 20:01:36]

Patchset #2 (id:20001) has been deleted

[teravest, 2015-04-01 16:04:58]

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

[teravest, 2015-04-01 17:18:13]

Agh, I sent this out a bit too soon. Investigating the 32-bit windows failure.

On Wed, Apr 1, 2015 at 10:04 AM,  <teravest@chromium.org> wrote:
> Reviewers: Nick Bray (chromium),
>
> Description:
> Enable Mojo/NaCl test on Windows.
>
> This can be enabled now that NACL_BUILD_SUBARCH is set correctly on Chrome
> windows builds.
>
> BUG=414804
>
> Please review this at https://codereview.chromium.org/1044953003/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+9, -17 lines):
>   M chrome/test/ppapi/ppapi_browsertest.cc
>   M components/nacl/loader/nacl_listener.cc
>
>
> Index: chrome/test/ppapi/ppapi_browsertest.cc
> diff --git a/chrome/test/ppapi/ppapi_browsertest.cc
> b/chrome/test/ppapi/ppapi_browsertest.cc
> index
>
8f6a5716de042f1960dfb7c12aca6bf7ee1a5a8e..8d5c92a2a4d9ad3690702532fb0c4ca5acd58224
> 100644
> --- a/chrome/test/ppapi/ppapi_browsertest.cc
> +++ b/chrome/test/ppapi/ppapi_browsertest.cc
> @@ -1452,17 +1452,11 @@ class MojoPPAPITest : public InProcessBrowserTest {
>    std::string result_;
>  };
>
> -#if defined(OS_POSIX)
> -#define MAYBE_MOJO(test_name) test_name
> -#else
> -#define MAYBE_MOJO(test_name) DISABLED_##test_name
> -#endif
> -
> -IN_PROC_BROWSER_TEST_F(MojoPPAPITest, MAYBE_MOJO(Mojo)) {
> +IN_PROC_BROWSER_TEST_F(MojoPPAPITest, Mojo) {
>    RunTest();
>  }
>
> -IN_PROC_BROWSER_TEST_F(MojoPPAPITest, MAYBE_MOJO(MojoFailsWithoutFlag)) {
> +IN_PROC_BROWSER_TEST_F(MojoPPAPITest, MojoFailsWithoutFlag) {
>    RunTestWithoutFlag();
>  }
>  #endif
> Index: components/nacl/loader/nacl_listener.cc
> diff --git a/components/nacl/loader/nacl_listener.cc
> b/components/nacl/loader/nacl_listener.cc
> index
>
424112eeb32ee08d63e1ef5e6b5cb9e8ed95df5d..89c4a9657e623818d448218464177a2cc6b4d00f
> 100644
> --- a/components/nacl/loader/nacl_listener.cc
> +++ b/components/nacl/loader/nacl_listener.cc
> @@ -428,13 +428,14 @@ void NaClListener::OnStart(const
> nacl::NaClStartParams& params) {
>    args->nexe_desc = NaClDescCreateWithFilePathMetadata(nexe_file,
>
> file_path_str.c_str());
>
> -#if defined(OS_POSIX)
>    if (params.enable_mojo) {
> -#if !defined(OS_MACOSX)
> -    // Don't call mojo::embedder::Init on Mac; it's already been called
> from
> -    // ChromeMain() (see chrome/app/chrome_exe_main_mac.cc).
> -    mojo::embedder::Init(make_scoped_ptr(
> -        new mojo::embedder::SimplePlatformSupport()));
> +#if defined(OS_LINUX)
> +    // On Linux, explicitly initialize the Mojo embedder here. On other
> +    // platforms, this initialization is handled via ContentMain(). On
> linux,
> +    // ContentMain() has not been called, as the this is the nacl_helper
> +    // process.
> +    mojo::embedder::Init(
> +        make_scoped_ptr(new mojo::embedder::SimplePlatformSupport()));
>  #endif
>      // InjectMojo adds a file descriptor to the process that allows Mojo
> calls
>      // to use an implementation defined outside the NaCl sandbox. See
> @@ -445,9 +446,6 @@ void NaClListener::OnStart(const nacl::NaClStartParams&
> params) {
>      // fails on any imc_sendmsg() call to make debugging easier.
>      InjectDisabledMojo(nap);
>    }
> -#else
> -  InjectDisabledMojo(nap);
> -#endif
>    // TODO(yusukes): Support pre-opening resource files.
>    CHECK(params.prefetched_resource_files.empty());
>
>
>

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

[teravest, 2015-04-01 22:58:18]

Okay, I think this is ready for review now.
I backed off on moving content::InitializeMojo somewhere public because I didn't
want to pull in too many dependencies; adding nonsfi support is going to require
a mojo nonsfi target, and I wasn't sure I wanted to do that for content/app as
well.

[Nick Bray (chromium), 2015-04-02 01:22:00]

Initial pass seems good, but it's late in the day and I want to look at it fresh
tomorrow.

https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
File components/nacl/loader/nacl_helper_win_64.cc (right):

https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
components/nacl/loader/nacl_helper_win_64.cc:68: make_scoped_ptr(new
mojo::embedder::SimplePlatformSupport()));
What about win32?  Are we guaranteed Mojo is initialized when we're built into
Chrome?

[teravest, 2015-04-02 14:06:32]

On 2015/04/02 01:22:00, Nick Bray (chromium) wrote:
> Initial pass seems good, but it's late in the day and I want to look at it
fresh
> tomorrow.
> 
>
https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
> File components/nacl/loader/nacl_helper_win_64.cc (right):
> 
>
https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
> components/nacl/loader/nacl_helper_win_64.cc:68: make_scoped_ptr(new
> mojo::embedder::SimplePlatformSupport()));
> What about win32?  Are we guaranteed Mojo is initialized when we're built into
> Chrome?

Yes, ContentMain calls content::InitializeMojo for win32 (and mac). The issue on
linux and win64 is that the nacl_helper process doesn't share initialization
code with the chrome binary.

[dmichael (off chromium), 2015-04-02 16:05:05]

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

[dmichael (off chromium), 2015-04-02 16:05:05]

Probably a good idea to update the description, since this does more than enable
the test. o/w lgtm fwiw

[Nick Bray (chromium), 2015-04-02 21:40:20]

LGTM

https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
components/nacl/loader/nacl_helper_linux.cc:465: // TODO(teravest): Enable mojo
for nonsfi.
What's the problem?  Is there an associated issue?

[teravest, 2015-04-02 22:20:25]

On 2015/04/02 21:40:20, Nick Bray (chromium) wrote:
> LGTM
> 
>
https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
> File components/nacl/loader/nacl_helper_linux.cc (right):
> 
>
https://codereview.chromium.org/1044953003/diff/140001/components/nacl/loader...
> components/nacl/loader/nacl_helper_linux.cc:465: // TODO(teravest): Enable
mojo
> for nonsfi.
> What's the problem?  Is there an associated issue?

I'll file one. We need to create a mojo_system_impl target for nonsfi that the
nonsfi nacl_helper target can depend on.

[teravest, 2015-04-03 16:18:05]

Committed patchset #6 (id:160001) manually as
75386dca6ebff785e62eb52472e660c757d5b2e8 (presubmit successful).

[Sergey Ulanov, 2015-04-03 19:07:10]

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

[Sergey Ulanov, 2015-04-03 19:07:11]

Looks like this change broke browser_tests on windows:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...

[teravest, 2015-04-03 19:25:23]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/1053313002/ by teravest@chromium.org.

The reason for reverting is: MojoFailsWithoutFlag fails on windows bots, as
reported by sergeyu (weirdly, this passed in the CQ).

[commit-bot: I haz the power, 2015-04-03 20:38:05]

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