Report exit status in debug stub, if tab is closed in chrome.

Report exit status in debug stub, if tab is closed in chrome.

This is essential for testing debug stub in chrome since we can't
determine if NaCl application is terminated successfully otherwise.

BUG= http://code.google.com/p/chromium/issues/detail?id=157312
TEST= NaClDebugStubRspTest.Empty in http://codereview.chromium.org/11236025/ CL.

[bsy, 2012-10-23 17:42:35]

http://codereview.chromium.org/11189140/diff/1/src/trusted/service_runtime/na...
File src/trusted/service_runtime/nacl_secure_service.c (right):

http://codereview.chromium.org/11189140/diff/1/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_secure_service.c:103: NaClLog(4,
"NaClSecureServiceThread: all channels closed, exiting.\n");
please add a comment for why this is needed, since w/o debug stub this isn't.

also, this function name suggests that it reports an exit status but will
return.  (which is indeed what it does.)  maybe add a comment that this will
cause the service runtime's main thread to exit as a side-effect?

i don't think we should use 0 here as the "exit status", since it conflates the
NaCl module's use of the exit syscall with value 0 with the command channel
closure, which is more akin to a kill or sigpipe.  we should probably encode it
in the wait status the same way that posix does with 8 bit exit status and
termsig/stopsig etc.

[Mark Seaborn, 2012-10-23 17:57:12]

I don't understand why you need this.  Can't your debug stub test just call
exit() or _exit() from untrusted code?

[halyavin, 2012-10-23 18:42:32]

On 2012/10/23 17:57:12, Mark Seaborn wrote:
> I don't understand why you need this.  Can't your debug stub test just call
> exit() or _exit() from untrusted code?

Maybe it can. But I don't want to change ppapi tests (ppapi/tests/test_empty.cc)
for this since they are not related to NaCl. And I think we should terminate
NaCl application on tab close gracefully, if we can. This will avoid warnings on
gdb side.

[halyavin, 2012-10-23 18:42:40]

http://codereview.chromium.org/11189140/diff/1/src/trusted/service_runtime/na...
File src/trusted/service_runtime/nacl_secure_service.c (right):

http://codereview.chromium.org/11189140/diff/1/src/trusted/service_runtime/na...
src/trusted/service_runtime/nacl_secure_service.c:103: NaClLog(4,
"NaClSecureServiceThread: all channels closed, exiting.\n");
On 2012/10/23 17:42:35, bsy wrote:
> please add a comment for why this is needed, since w/o debug stub this isn't.
> 
> also, this function name suggests that it reports an exit status but will
> return.  (which is indeed what it does.)  maybe add a comment that this will
> cause the service runtime's main thread to exit as a side-effect?
I added a comment.

> 
> i don't think we should use 0 here as the "exit status", since it conflates
the
> NaCl module's use of the exit syscall with value 0 with the command channel
> closure, which is more akin to a kill or sigpipe.  we should probably encode
it
> in the wait status the same way that posix does with 8 bit exit status and
> termsig/stopsig etc.

This is normal scenario in browser. So I don't think we need to send non-zero
here.
 
FYI, debug stub sends only 8 bits of exit code.

[Mark Seaborn, 2012-10-23 18:49:59]

On 23 October 2012 11:42, <halyavin@google.com> wrote:

> On 2012/10/23 17:57:12, Mark Seaborn wrote:
>
>> I don't understand why you need this.  Can't your debug stub test just
>> call
>> exit() or _exit() from untrusted code?
>>
>
> Maybe it can. But I don't want to change ppapi tests
> (ppapi/tests/test_empty.cc)
> for this since they are not related to NaCl.


But test_empty.cc is something you added yourself for testing NaCl
debugging functionality.  I think you should prefer changing the test code
over changing trusted code in this case.

And I think we should terminate
> NaCl application on tab close gracefully, if we can. This will avoid
> warnings on
> gdb side.
>

What warnings/errors do you get in GDB with and without this change?

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2012-10-23 18:56:00]

On 23 October 2012 11:42, <halyavin@google.com> wrote:

> i don't think we should use 0 here as the "exit status", since it
>> conflates the
>>
> NaCl module's use of the exit syscall with value 0 with the command channel
>>
> closure, which is more akin to a kill or sigpipe.  we should probably
>> encode it
>>
> in the wait status the same way that posix does with 8 bit exit status and
>>
> termsig/stopsig etc.
>>
>
> This is normal scenario in browser. So I don't think we need to send
> non-zero here.
>
> FYI, debug stub sends only 8 bits of exit code.
>

I agree with Bennet here.  Sending a zero exit status for a forcible
termination is not appropriate.  It will be misleading for anyone seeing
this exit status using the debug stub or seeing it Chromium's logs, or in
other cases where we might use the exit status in future.  You should only
get an exit status of zero if user code has called _exit(0) explicitly.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2012-10-23 19:07:48]

On 23 October 2012 12:03, Andrey Khalyavin <halyavin@google.com> wrote:

> This is not a forcible termination, this happens when tab is closed.
> Closing tab is not an error. It would be confusing to see a non-zero exit
> status for it.


That *is* forcible termination.  The sandboxed NaCl process is being
terminated, asynchronously, by outside forces without the sandboxed process
being notified.  It is just like kill() in Unix.  kill() is not an error
either.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[halyavin, 2012-10-24 08:49:01]

Changing in gdb output.
Before:
Remote connection closed
After:
[Inferior 1 (Remote target) exited normally]


2012/10/23 Mark Seaborn <mseaborn@chromium.org>

> On 23 October 2012 12:03, Andrey Khalyavin <halyavin@google.com> wrote:
>
>> This is not a forcible termination, this happens when tab is closed.
>> Closing tab is not an error. It would be confusing to see a non-zero exit
>> status for it.
>
>
> That *is* forcible termination.  The sandboxed NaCl process is being
> terminated, asynchronously, by outside forces without the sandboxed process
> being notified.  It is just like kill() in Unix.  kill() is not an error
> either.
>
> Mark
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[halyavin, 2012-10-25 11:26:05]

On 2012/10/23 17:57:12, Mark Seaborn wrote:
> I don't understand why you need this.  Can't your debug stub test just call
> exit() or _exit() from untrusted code?

I just made an experiment. It doesn't help since exit is not instantaneous. Test
can kill tab and hence calls NaClExit(0) before exit code is processed in main
thread. And this does happens in practice on release builds.

[halyavin, 2012-10-25 11:37:38]

I investigated how javascript deals with exit codes.

Javascript doesn't receive any messages when NaCl module exits. Exit code is
determined by exitCode property of the embed object. This property is set by
ReportExitStatus via SRPC i.e. in our situation exit code can't be reported.