Simple shell that loads a dll and calls an entrypoint function passing in a handle to a pipe create…

Simple shell that loads a dll and calls an entrypoint function passing in a handle to a pipe created by the shell app.

To achieve this I had to make mojo_system a <(component) so sample_app.dll could link against it. Trung, Darin tells me you had a different idea about how to achieve this. Consider this CL a starting point for the discussion :-)

R=darin@chromium.org
BUG=

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

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

[Ben Goodger (Google), 2013-10-03 17:45:18]

https://codereview.chromium.org/25895002/diff/1/mojo/mojo.gyp
File mojo/mojo.gyp (right):

https://codereview.chromium.org/25895002/diff/1/mojo/mojo.gyp#newcode72
mojo/mojo.gyp:72: 'SYSTEM_IMPLEMENTATION',
MOJO_SYSTEM_IMPLEMENTATION?

https://codereview.chromium.org/25895002/diff/1/mojo/shell/sample_app.cc
File mojo/shell/sample_app.cc (right):

https://codereview.chromium.org/25895002/diff/1/mojo/shell/sample_app.cc#newc...
mojo/shell/sample_app.cc:49: // Failure...
a typical meme in this code. can we have a discussion about error handling?

https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h#newco...
mojo/system/core_impl.h:28: class SYSTEM_EXPORT CoreImpl {
needed so shell's main() can init.

[darin (slow to review), 2013-10-04 21:47:13]

https://codereview.chromium.org/25895002/diff/1/mojo/public/system/core.h
File mojo/public/system/core.h (right):

https://codereview.chromium.org/25895002/diff/1/mojo/public/system/core.h#new...
mojo/public/system/core.h:12: #include "mojo/public/system/system_export.h"
looks like this file is missing from the CL. i agree that the macro should be
prefixed with MOJO_.

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc
File mojo/shell/app_container.cc (right):

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc#n...
mojo/shell/app_container.cc:31: goto completed;
in these other error cases, we'll want to make sure the DLL doesn't leak. or
perhaps it is OK to leak it and just let the child process exit (once we have a
child process).

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc#n...
mojo/shell/app_container.cc:40: 
i guess there is a TODO here for error handling.

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc#n...
mojo/shell/app_container.cc:69: base::MessageLoop::current(),
there's an effort to move away from MessageLoop and use separate primitives for
identifying the current TaskRunner and the act of running and stopping the
message loop.  see base::ThreadTaskRunnerHandle and base::RunLoop.

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.h
File mojo/shell/app_container.h (right):

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.h#ne...
mojo/shell/app_container.h:35: MojoHandle shell_handle_;
nit: might want to use the C++ version of this: mojo::Handle. it is type-safe.

https://codereview.chromium.org/25895002/diff/1/mojo/shell/sample_app.cc
File mojo/shell/sample_app.cc (right):

https://codereview.chromium.org/25895002/diff/1/mojo/shell/sample_app.cc#newc...
mojo/shell/sample_app.cc:49: // Failure...
On 2013/10/03 17:45:18, Ben Goodger (Google) wrote:
> a typical meme in this code. can we have a discussion about error handling?

yeah, we should probably figure out a good canonical way of using these
primitives. porting MessageLoop to mojo is probably part of that.

https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h#newco...
mojo/system/core_impl.h:28: class SYSTEM_EXPORT CoreImpl {
On 2013/10/03 17:45:18, Ben Goodger (Google) wrote:
> needed so shell's main() can init.

yeah, with this code broken out as a separate DLL, we now need to define some
private exports for using mojo_system.dll from the trusted process.  hmmm...

[Ben Goodger (Google), 2013-10-07 23:48:43]

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc
File mojo/shell/app_container.cc (right):

https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc#n...
mojo/shell/app_container.cc:69: base::MessageLoop::current(),
On 2013/10/04 21:47:14, darin wrote:
> there's an effort to move away from MessageLoop and use separate primitives
for
> identifying the current TaskRunner and the act of running and stopping the
> message loop.  see base::ThreadTaskRunnerHandle and base::RunLoop.

I can easily pass a threadtaskrunner to my function... but I don't see how to
avoid messageloop here completely since base::Thread provides a message loop,
not a runloop. What am I missing?

https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h
File mojo/system/core_impl.h (right):

https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h#newco...
mojo/system/core_impl.h:28: class SYSTEM_EXPORT CoreImpl {
On 2013/10/04 21:47:14, darin wrote:
> On 2013/10/03 17:45:18, Ben Goodger (Google) wrote:
> > needed so shell's main() can init.
> 
> yeah, with this code broken out as a separate DLL, we now need to define some
> private exports for using mojo_system.dll from the trusted process.  hmmm...

I can think of a variety of ways to make this more obscure, but none better than
the existing presence of a public/ dir and tight DEPS rules.

[darin (slow to review), 2013-10-08 03:50:56]

On Mon, Oct 7, 2013 at 4:48 PM, <ben@chromium.org> wrote:

>
> https://codereview.chromium.**org/25895002/diff/1/mojo/**
>
shell/app_container.cc<https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc>
> File mojo/shell/app_container.cc (right):
>
> https://codereview.chromium.**org/25895002/diff/1/mojo/**
>
shell/app_container.cc#**newcode69<https://codereview.chromium.org/25895002/diff/1/mojo/shell/app_container.cc#newcode69>
> mojo/shell/app_container.cc:**69: base::MessageLoop::current(),
> On 2013/10/04 21:47:14, darin wrote:
>
>> there's an effort to move away from MessageLoop and use separate
>>
> primitives for
>
>> identifying the current TaskRunner and the act of running and stopping
>>
> the
>
>> message loop.  see base::ThreadTaskRunnerHandle and base::RunLoop.
>>
>
> I can easily pass a threadtaskrunner to my function... but I don't see
> how to avoid messageloop here completely since base::Thread provides a
> message loop, not a runloop. What am I missing?


Hmm, things are in a bit of a transitional state. You can get the
MessageLoopProxy from the Thread via the message_loop_proxy() method. That
returns a MessageLoopProxy which is a SingleThreadTaskRunner.

The calls to base::MessageLoop::current()->Run() and Quit() can be replaced
with base::RunLoop::Run() and Quit(). The only trick there is that you need
an instance of base::RunLoop.  So, you might for example
make AppContainer::LaunchApp() take a base::Callback that gets invoked from
AppCompleted. That Callback would be responsible for calling Quit on the
RunLoop. The idea is to keep the calls to RunLoop's Run() and Quit()
methods in closer proximity.



>
>
> https://codereview.chromium.**org/25895002/diff/1/mojo/**
>
system/core_impl.h<https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h>
> File mojo/system/core_impl.h (right):
>
> https://codereview.chromium.**org/25895002/diff/1/mojo/**
>
system/core_impl.h#newcode28<https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h#newcode28>
> mojo/system/core_impl.h:28: class SYSTEM_EXPORT CoreImpl {
> On 2013/10/04 21:47:14, darin wrote:
>
>> On 2013/10/03 17:45:18, Ben Goodger (Google) wrote:
>> > needed so shell's main() can init.
>>
>
>  yeah, with this code broken out as a separate DLL, we now need to
>>
> define some
>
>> private exports for using mojo_system.dll from the trusted process.
>>
> hmmm...
>
> I can think of a variety of ways to make this more obscure, but none
> better than the existing presence of a public/ dir and tight DEPS rules.
>

Yeah, I haven't given much thought to designing the embedding API for the
system DLL. Perhaps Trung has.

-Darin



>
>
https://codereview.chromium.**org/25895002/<https://codereview.chromium.org/2...
>

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

[vtl, 2013-10-08 17:03:51]

On Mon, Oct 7, 2013 at 8:50 PM, Darin Fisher <darin@chromium.org> wrote:

>
> On Mon, Oct 7, 2013 at 4:48 PM, <ben@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/25895002/diff/1/mojo/**
>>
system/core_impl.h<https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h>
>> File mojo/system/core_impl.h (right):
>>
>> https://codereview.chromium.**org/25895002/diff/1/mojo/**
>>
system/core_impl.h#newcode28<https://codereview.chromium.org/25895002/diff/1/mojo/system/core_impl.h#newcode28>
>> mojo/system/core_impl.h:28: class SYSTEM_EXPORT CoreImpl {
>> On 2013/10/04 21:47:14, darin wrote:
>>
>>> On 2013/10/03 17:45:18, Ben Goodger (Google) wrote:
>>> > needed so shell's main() can init.
>>>
>>
>>  yeah, with this code broken out as a separate DLL, we now need to
>>>
>> define some
>>
>>> private exports for using mojo_system.dll from the trusted process.
>>>
>> hmmm...
>>
>> I can think of a variety of ways to make this more obscure, but none
>> better than the existing presence of a public/ dir and tight DEPS rules.
>>
>
> Yeah, I haven't given much thought to designing the embedding API for the
> system DLL. Perhaps Trung has.
>

I can't think of anything better. Hopefully, we can limit the size of the
embedding API (there's init and the "native" implementations of the public
API). I guess that trusted processes would use (the native) base, etc.
directly.

Maybe we should have different export macros for the embedding API proper
and things that would also be made available to untrusted code (e.g., under
NaCl, presumably the embedder would provide a NaCl system call interface
and C library stub code to access the public API)....

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

[Ben Goodger (Google), 2013-10-08 17:06:48]

Updated.

[darin (slow to review), 2013-10-08 21:30:05]

LGTM

https://codereview.chromium.org/25895002/diff/17001/mojo/shell/app_container.cc
File mojo/shell/app_container.cc (right):

https://codereview.chromium.org/25895002/diff/17001/mojo/shell/app_container....
mojo/shell/app_container.cc:45: mojo::Close(app_handle);
nit: you could leave off the mojo:: if you wanted since you are already inside
the mojo namespace. ditto for the remainder of this file.

[Ben Goodger (Google), 2013-10-08 21:43:52]

Committed patchset #5 manually as r227604 (presubmit successful).

[Ben Goodger (Google), 2013-10-10 16:53:24]

I sprinkled a minimal set of exports in to make mojo_system_unittests link. We
will probably need more, but I'll leave that for trung to figure out ;D

[darin (slow to review), 2013-10-10 17:54:39]

LGTM

[Ben Goodger (Google), 2013-10-10 20:52:26]

Committed patchset #12 manually as r227983 (presubmit successful).

[Nico, 2013-10-12 17:03:25]

https://codereview.chromium.org/25895002/diff/65001/build/all.gyp
File build/all.gyp (right):

https://codereview.chromium.org/25895002/diff/65001/build/all.gyp#newcode45
build/all.gyp:45: '../mojo/mojo.gyp:*',
One side effect of having this dependency in all and nowhere else is that this
target is built on almost no bots, but it _is_ build on the chromium.chrome mac
bot and the mac clobber bot, both of which are on the main waterfall. So mojo
can turn the tree red, but trybots won't warn you, and you'll only learn about
it after several hours as these two bots are very slow.

(Example: I broke mojo compilation like so
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Mac/bu...
and didn't realize until the next morning)