Show more different NaCl loading errors.

Show more different NaCl loading errors.

Also refactor NaCl launch message to pass output parameters in a single
structure.

I changed nacl::FileDescriptor type on Windows from int to HANDLE to make it
compatible with IPC::PlatformFileForTransit. The only ways nacl::FileDescriptor
is currently used are its initialization, passing in IPC message and in
nacl::ToNativeHandle function. The behaviour of nacl::ToNativeHandle haven't
changed. IPC doesn't have special handling for integers and it only truncates
HANDLEs to 32-bit (so that they can be passed between 32-bit and 64-bit
processes) on Windows. So the change shouldn't affect any existing users of
nacl::FileDescriptor type.

TEST= bots + changing code to produce an error on normal code path
BUG=259333
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212913

[halyavin, 2013-07-10 08:09:58]

Justin Schuh: I need approval for nacl_host_messages.h changes.
dmichael: I need approval for ppapi/* changes.
sky: I need approval for changes in nacl_types.h and nacl_types.cc since they
haven't file specific OWNERS.

[dmichael (off chromium), 2013-07-10 17:07:19]

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:542: SendErrorToRenderer("Failed
to add NaCl process PID");
nit: let's make the capitalization consistent. Here, you're using upper-case,
elsewhere you are using lowercase (not counting DuplicateHandle, of course)

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:549:
NaClHostMsg_LaunchNaCl::WriteReplyParams(
For the sake of making the code more self-documenting, what about changing
SendErrorToRenderer to SendReplyToRenderer, and using it here? And that method
can have something like:
// This method must only be called once.
DCHECK(nacl_host_message_filter_);
DCHECK(reply_msg_);

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:555: std::string());
an inline comment might be nice "std::string() /* error_message */);"

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559:
internal_->socket_for_renderer = NACL_INVALID_HANDLE;
^^^ You're currently not doing this for the error case; is that on purpose?

https://codereview.chromium.org/18045007/diff/23001/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/18045007/diff/23001/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:84: *error_message =
ppapi::StringVar::StringToPPVar("");
The PP_Var you're getting has a ref-count of 1, so right now you're leaking this
reference if you reset error_message. You can use ScopedPPVar, but it might be
easier to either (a) leave error_message untouched if there's no error, or (b)
use PP_MakeUndefined if there's no error.

https://codereview.chromium.org/18045007/diff/23001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc (right):

https://codereview.chromium.org/18045007/diff/23001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc:32: struct
PP_Var var;
struct is unnecessary here

https://codereview.chromium.org/18045007/diff/23001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc:43:
*error_message = pp::Var(var).AsString();
You need to use the PassRef constructor, or you'll leak a reference to the
PP_Var.

[jschuh, 2013-07-10 19:45:45]

ipc security rubberstamp lgtm. no attack surface change, just moving moving from
function parameters to an ipc struct.

[halyavin, 2013-07-11 09:03:35]

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:542: SendErrorToRenderer("Failed
to add NaCl process PID");
On 2013/07/10 17:07:20, dmichael wrote:
> nit: let's make the capitalization consistent. Here, you're using upper-case,
> elsewhere you are using lowercase (not counting DuplicateHandle, of course)

Done.

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:549:
NaClHostMsg_LaunchNaCl::WriteReplyParams(
On 2013/07/10 17:07:20, dmichael wrote:
> For the sake of making the code more self-documenting, what about changing
> SendErrorToRenderer to SendReplyToRenderer, and using it here? And that method
> can have something like:
> // This method must only be called once.
> DCHECK(nacl_host_message_filter_);
> DCHECK(reply_msg_);
We are passing actual data here instead of empty nacl::NaClLaunchResult in
SendErrorToRenderer.

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:555: std::string());
On 2013/07/10 17:07:20, dmichael wrote:
> an inline comment might be nice "std::string() /* error_message */);"

Done.

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559:
internal_->socket_for_renderer = NACL_INVALID_HANDLE;
On 2013/07/10 17:07:20, dmichael wrote:
> ^^^ You're currently not doing this for the error case; is that on purpose?

Yes, this is done on purpose. The handle is set to NACL_INVALID_HANDLE since it
would be closed while sending to another process and so we should not close it
in destructor. Since we don't send this handle on error, we should left it as is
and let destructor to close it.

I realized that there are resource leaks here while investigating this but I
would like to fix them in a separate CL.

https://codereview.chromium.org/18045007/diff/23001/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/18045007/diff/23001/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:84: *error_message =
ppapi::StringVar::StringToPPVar("");
On 2013/07/10 17:07:20, dmichael wrote:
> The PP_Var you're getting has a ref-count of 1, so right now you're leaking
this
> reference if you reset error_message. You can use ScopedPPVar, but it might be
> easier to either (a) leave error_message untouched if there's no error, or (b)
> use PP_MakeUndefined if there's no error.

Done.

https://codereview.chromium.org/18045007/diff/23001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc (right):

https://codereview.chromium.org/18045007/diff/23001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc:32: struct
PP_Var var;
On 2013/07/10 17:07:20, dmichael wrote:
> struct is unnecessary here

Done.

https://codereview.chromium.org/18045007/diff/23001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.cc:43:
*error_message = pp::Var(var).AsString();
On 2013/07/10 17:07:20, dmichael wrote:
> You need to use the PassRef constructor, or you'll leak a reference to the
> PP_Var.

Done.

[halyavin, 2013-07-11 12:49:44]

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559:
internal_->socket_for_renderer = NACL_INVALID_HANDLE;
The handle leak fix is here: https://codereview.chromium.org/18474005

[dmichael (off chromium), 2013-07-11 16:12:59]

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:549:
NaClHostMsg_LaunchNaCl::WriteReplyParams(
On 2013/07/11 09:03:36, halyavin wrote:
> On 2013/07/10 17:07:20, dmichael wrote:
> > For the sake of making the code more self-documenting, what about changing
> > SendErrorToRenderer to SendReplyToRenderer, and using it here? And that
method
> > can have something like:
> > // This method must only be called once.
> > DCHECK(nacl_host_message_filter_);
> > DCHECK(reply_msg_);
> We are passing actual data here instead of empty nacl::NaClLaunchResult in
> SendErrorToRenderer.
I know, I'm suggesting you add an argument to SendReplyToRenderer. In the Error
cases, you can pass a default NaClLaunchResult (or maybe still have
SendErrorToRenderer that wraps SendReplyToRenderer if you prefer). There's
enough behavior involved that we should put that code in 1 place (you have to
send the message and clear 2 members).

https://codereview.chromium.org/18045007/diff/23001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559:
internal_->socket_for_renderer = NACL_INVALID_HANDLE;
On 2013/07/11 12:49:44, halyavin wrote:
> The handle leak fix is here: https://codereview.chromium.org/18474005
> 
If we're sending a message to the other process on error anyway, maybe it would
be simpler to send the handle and let that process always close it?

[dmichael (off chromium), 2013-07-12 18:28:34]

lgtm, but I assume you'll need to wait on the LaunchSelLdr threading fix

[sky, 2013-07-15 18:28:31]

nacl_types LGTM

[Mark Seaborn, 2013-07-16 02:02:52]

https://codereview.chromium.org/18045007/diff/23003/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/23003/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:292: SendErrorToRenderer("could
not find all the resources needed"
Are these error messages displayed in the console with a prefix?  I don't see a
prefix being added.  If not, the first letter should be capitalised.  Even if
they're prefixed, capitalising doesn't hurt, so please do it for all these
messages.

https://codereview.chromium.org/18045007/diff/23003/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:431: SendErrorToRenderer("could
not resolve module");
How about: "Could not get nacl64.exe path"?

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types.cc
File chrome/common/nacl_types.cc (right):

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types....
chrome/common/nacl_types.cc:57: : imc_channel_handle(-1),
0 would be safer than -1.  -1 is INVALID_HANDLE_VALUE but it's also the same as
GetCurrentProcess().  See
http://blogs.msdn.com/b/oldnewthing/archive/2004/03/02/82639.aspx

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:745:
params.error_info->SetReport(ERROR_SEL_LDR_LAUNCH, error_message);
This error gets reported to untrusted Javascript via lastError, doesn't it?  I'm
not sure that is a good idea.  It will expose platform-specific errors like
messages about DuplicateHandle().  There's a risk that error messages are
unintentionally changed in the future to leak sensitive information.

What error messages are exposed to Javascript at the moment?  Maybe we could
reduce the set, or at least not increase it?

Nick has worked on this error-handling code before.  Maybe he can comment?

[halyavin, 2013-07-16 08:04:35]

https://codereview.chromium.org/18045007/diff/23003/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/23003/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:292: SendErrorToRenderer("could
not find all the resources needed"
On 2013/07/16 02:02:53, Mark Seaborn wrote:
> Are these error messages displayed in the console with a prefix?  I don't see
a
> prefix being added.  If not, the first letter should be capitalised.  Even if
> they're prefixed, capitalising doesn't hurt, so please do it for all these
> messages.

Yes, they are prefixed with "NaCl process launch failed: " as before in the
SendErrorToRenderer function.

https://codereview.chromium.org/18045007/diff/23003/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:431: SendErrorToRenderer("could
not resolve module");
On 2013/07/16 02:02:53, Mark Seaborn wrote:
> How about: "Could not get nacl64.exe path"?

Done.

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types.cc
File chrome/common/nacl_types.cc (right):

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types....
chrome/common/nacl_types.cc:57: : imc_channel_handle(-1),
On 2013/07/16 02:02:53, Mark Seaborn wrote:
> 0 would be safer than -1.  -1 is INVALID_HANDLE_VALUE but it's also the same
as
> GetCurrentProcess().  See
> http://blogs.msdn.com/b/oldnewthing/archive/2004/03/02/82639.aspx

I know, but IPC::GetFileHandleForProcess in ipc/ipc_platform_file.cc uses -1 for
invalid handles and the normal NaCl loading would not work, if ipc will start
duplicate windows handles in IPC messages. So it looks like the intended way to
pass invalid handles over IPC for me.

May be a better alternative is to use
  #include "ipc/ipc_platform_file.h"
  ...
  : imc_channel_handle(IPC::InvalidPlatformFileForTransit()),
We will not even need ifdefs then.

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:745:
params.error_info->SetReport(ERROR_SEL_LDR_LAUNCH, error_message);
On 2013/07/16 02:02:53, Mark Seaborn wrote:
> This error gets reported to untrusted Javascript via lastError, doesn't it? 
I'm
> not sure that is a good idea.  It will expose platform-specific errors like
> messages about DuplicateHandle().  There's a risk that error messages are
> unintentionally changed in the future to leak sensitive information.
Looks like you are right.
> 
> What error messages are exposed to Javascript at the moment?  Maybe we could
> reduce the set, or at least not increase it?
Do you want to have different errors printed to the javascript console and set
to lastError?
> 
> Nick has worked on this error-handling code before.  Maybe he can comment?

[Nick Bray (chromium), 2013-07-16 19:29:55]

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:310: SendErrorToRenderer("could
not create a socket pair");
These error messages are pretty vague.  That was semi-acceptable when they were
only printed to the console, but if we're exposing them to JS they need to be
improved.  Maybe "internal error - %s while launching Native Client process"?

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:396: SendErrorToRenderer("could
not create channel");
What kind of channel?

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:431: SendErrorToRenderer("could
not get nacl64.exe path");
could not get path to nacl64.exe

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:516: SendErrorToRenderer("failed
to add NaCl process PID");
Add it to what?  To where?

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:534:
SendErrorToRenderer("DuplicateHandle() failed");
No idea how to explain this one better.

[Mark Seaborn, 2013-07-17 05:29:57]

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types.cc
File chrome/common/nacl_types.cc (right):

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types....
chrome/common/nacl_types.cc:57: : imc_channel_handle(-1),
On 2013/07/16 08:04:35, halyavin wrote:
> On 2013/07/16 02:02:53, Mark Seaborn wrote:
> > 0 would be safer than -1.  -1 is INVALID_HANDLE_VALUE but it's also the
> > same as GetCurrentProcess().  See
> > http://blogs.msdn.com/b/oldnewthing/archive/2004/03/02/82639.aspx

> May be a better alternative is to use
>   #include "ipc/ipc_platform_file.h"
>   ...
>   : imc_channel_handle(IPC::InvalidPlatformFileForTransit()),
> We will not even need ifdefs then.

Yes, that's much better than using a numeric constant.

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:745:
params.error_info->SetReport(ERROR_SEL_LDR_LAUNCH, error_message);
On 2013/07/16 08:04:35, halyavin wrote:
> On 2013/07/16 02:02:53, Mark Seaborn wrote:
> > This error gets reported to untrusted Javascript via lastError, doesn't it? 
> > I'm
> > not sure that is a good idea.  It will expose platform-specific errors like
> > messages about DuplicateHandle().  There's a risk that error messages are
> > unintentionally changed in the future to leak sensitive information.
> Looks like you are right.
> > 
> > What error messages are exposed to Javascript at the moment?  Maybe we could
> > reduce the set, or at least not increase it?
>
> Do you want to have different errors printed to the javascript console and set
> to lastError?

Yes, I think so, to avoid accidentally exposing sensitive information, and to
avoid apps depending on specific messages.

You'd only need to hide the new errors you're adding from Javascript.

[halyavin, 2013-07-17 13:12:16]

PTAL

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types.cc
File chrome/common/nacl_types.cc (right):

https://codereview.chromium.org/18045007/diff/23003/chrome/common/nacl_types....
chrome/common/nacl_types.cc:57: : imc_channel_handle(-1),
On 2013/07/17 05:29:58, Mark Seaborn wrote:
> On 2013/07/16 08:04:35, halyavin wrote:
> > On 2013/07/16 02:02:53, Mark Seaborn wrote:
> > > 0 would be safer than -1.  -1 is INVALID_HANDLE_VALUE but it's also the
> > > same as GetCurrentProcess().  See
> > > http://blogs.msdn.com/b/oldnewthing/archive/2004/03/02/82639.aspx
> 
> > May be a better alternative is to use
> >   #include "ipc/ipc_platform_file.h"
> >   ...
> >   : imc_channel_handle(IPC::InvalidPlatformFileForTransit()),
> > We will not even need ifdefs then.
> 
> Yes, that's much better than using a numeric constant.

Done.

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/18045007/diff/23003/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:745:
params.error_info->SetReport(ERROR_SEL_LDR_LAUNCH, error_message);
On 2013/07/17 05:29:58, Mark Seaborn wrote:
> On 2013/07/16 08:04:35, halyavin wrote:
> > On 2013/07/16 02:02:53, Mark Seaborn wrote:
> > > This error gets reported to untrusted Javascript via lastError, doesn't
it? 
> > > I'm
> > > not sure that is a good idea.  It will expose platform-specific errors
like
> > > messages about DuplicateHandle().  There's a risk that error messages are
> > > unintentionally changed in the future to leak sensitive information.
> > Looks like you are right.
> > > 
> > > What error messages are exposed to Javascript at the moment?  Maybe we
could
> > > reduce the set, or at least not increase it?
> >
> > Do you want to have different errors printed to the javascript console and
set
> > to lastError?
> 
> Yes, I think so, to avoid accidentally exposing sensitive information, and to
> avoid apps depending on specific messages.
> 
> You'd only need to hide the new errors you're adding from Javascript.

Done.

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:310: SendErrorToRenderer("could
not create a socket pair");
On 2013/07/16 19:29:55, Nick Bray (chromium) wrote:
> These error messages are pretty vague.  That was semi-acceptable when they
were
> only printed to the console, but if we're exposing them to JS they need to be
> improved.  Maybe "internal error - %s while launching Native Client process"?

I update this message a bit. Also, now this messages are printed only to
javascript console.

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:396: SendErrorToRenderer("could
not create channel");
On 2013/07/16 19:29:55, Nick Bray (chromium) wrote:
> What kind of channel?

Done.

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:431: SendErrorToRenderer("could
not get nacl64.exe path");
On 2013/07/16 19:29:55, Nick Bray (chromium) wrote:
> could not get path to nacl64.exe

Done.

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:516: SendErrorToRenderer("failed
to add NaCl process PID");
On 2013/07/16 19:29:55, Nick Bray (chromium) wrote:
> Add it to what?  To where?

Done.

https://codereview.chromium.org/18045007/diff/144001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:534:
SendErrorToRenderer("DuplicateHandle() failed");
On 2013/07/16 19:29:55, Nick Bray (chromium) wrote:
> No idea how to explain this one better.

Done.

[Mark Seaborn, 2013-07-17 18:21:49]

https://codereview.chromium.org/18045007/diff/159001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/18045007/diff/159001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:310: SendErrorToRenderer("internal
error: could not create a channel"
This error message is actually misleading.  NaClSocketPair() does not itself
create a channel between NaCl and renderer.  I disagree with Nick somewhat here.
 "NaClSocketPair() failed" would be a more straightforward error here.  These
are all unexpected failure cases.  Their purpose is to be understandable to
developers of NaCl and little more.

https://codereview.chromium.org/18045007/diff/159001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:397: SendErrorToRenderer("could
not create IPC channel to NaCl process");
Also misleading since it does not connect the IPC channel.  How about
"CreateChannel() failed"?

https://codereview.chromium.org/18045007/diff/159001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_process_host.cc:517: SendErrorToRenderer("failed
to register NaCl process in sandbox system");
How about "BrokerAddTargetPeer() failed"?

https://codereview.chromium.org/18045007/diff/159001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://codereview.chromium.org/18045007/diff/159001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin_error.h:121: void
SetReport(PluginErrorCode error_code, const std::string& message,
Please avoid function overloading when possible: it makes it hard to find call
sites by grepping.  Can you give this variant a different name?  e.g.
SetReportWithTrustedError() or SetReportWithConsoleOnlyError().

There should also be a comment explaining why some errors are only logged to the
console.

https://codereview.chromium.org/18045007/diff/159001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin_error.h:125: console_message_ =
console_message;
Are you going to concatenate the messages like we discussed on IM?

[halyavin, 2013-07-18 09:32:21]

https://chromiumcodereview.appspot.com/18045007/diff/159001/chrome/browser/na...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/18045007/diff/159001/chrome/browser/na...
chrome/browser/nacl_host/nacl_process_host.cc:310: SendErrorToRenderer("internal
error: could not create a channel"
On 2013/07/17 18:21:49, Mark Seaborn wrote:
> This error message is actually misleading.  NaClSocketPair() does not itself
> create a channel between NaCl and renderer.  I disagree with Nick somewhat
here.
>  "NaClSocketPair() failed" would be a more straightforward error here.  These
> are all unexpected failure cases.  Their purpose is to be understandable to
> developers of NaCl and little more.

Done.

https://chromiumcodereview.appspot.com/18045007/diff/159001/chrome/browser/na...
chrome/browser/nacl_host/nacl_process_host.cc:397: SendErrorToRenderer("could
not create IPC channel to NaCl process");
On 2013/07/17 18:21:49, Mark Seaborn wrote:
> Also misleading since it does not connect the IPC channel.  How about
> "CreateChannel() failed"?

Done.

https://chromiumcodereview.appspot.com/18045007/diff/159001/chrome/browser/na...
chrome/browser/nacl_host/nacl_process_host.cc:517: SendErrorToRenderer("failed
to register NaCl process in sandbox system");
On 2013/07/17 18:21:49, Mark Seaborn wrote:
> How about "BrokerAddTargetPeer() failed"?

Done.

https://chromiumcodereview.appspot.com/18045007/diff/159001/ppapi/native_clie...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://chromiumcodereview.appspot.com/18045007/diff/159001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:121: void
SetReport(PluginErrorCode error_code, const std::string& message,
On 2013/07/17 18:21:49, Mark Seaborn wrote:
> Please avoid function overloading when possible: it makes it hard to find call
> sites by grepping.  Can you give this variant a different name?  e.g.
> SetReportWithTrustedError() or SetReportWithConsoleOnlyError().
> 
> There should also be a comment explaining why some errors are only logged to
the
> console.

Done.

https://chromiumcodereview.appspot.com/18045007/diff/159001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:125: console_message_ =
console_message;
On 2013/07/17 18:21:49, Mark Seaborn wrote:
> Are you going to concatenate the messages like we discussed on IM?

Done.

[Mark Seaborn, 2013-07-18 22:15:48]

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
File chrome/common/nacl_types.h (right):

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
chrome/common/nacl_types.h:28: // We assume that HANDLE always uses less than 32
bits
This comment no longer matches the following line.

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
chrome/common/nacl_types.h:29: typedef HANDLE FileDescriptor;
This change requires an explanation in the commit message.

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
chrome/common/nacl_types.h:31: return reinterpret_cast<HANDLE>(desc);
This can be just "return desc" now.

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:121: // console_message is
a part of error that is logged to javascript console
"javascript" -> "Javascript" or "JavaScript"

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:122: // but is not
reported to javascript on the page.  This is need to report
More specifically: "not reported to JavaScript via .lastError".

"need" -> "needed".  Actually, "used" would be better.

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:129: console_message_ =
message + console_message;
Shouldn't there be some punctuation between the messages, like a colon?  What's
an example of the output here?  Isn't this going to output something like
"ServiceRuntime: failed to startNaClSocketPair() failed"?

[halyavin, 2013-07-19 09:06:20]

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
File chrome/common/nacl_types.h (right):

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
chrome/common/nacl_types.h:28: // We assume that HANDLE always uses less than 32
bits
On 2013/07/18 22:15:49, Mark Seaborn wrote:
> This comment no longer matches the following line.

Done.

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
chrome/common/nacl_types.h:29: typedef HANDLE FileDescriptor;
On 2013/07/18 22:15:49, Mark Seaborn wrote:
> This change requires an explanation in the commit message.

Done.

https://chromiumcodereview.appspot.com/18045007/diff/200001/chrome/common/nac...
chrome/common/nacl_types.h:31: return reinterpret_cast<HANDLE>(desc);
On 2013/07/18 22:15:49, Mark Seaborn wrote:
> This can be just "return desc" now.

Done.

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:121: // console_message is
a part of error that is logged to javascript console
On 2013/07/18 22:15:49, Mark Seaborn wrote:
> "javascript" -> "Javascript" or "JavaScript"

Done.

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:122: // but is not
reported to javascript on the page.  This is need to report
On 2013/07/18 22:15:49, Mark Seaborn wrote:
> More specifically: "not reported to JavaScript via .lastError".
> 
> "need" -> "needed".  Actually, "used" would be better.

Done.

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:129: console_message_ =
message + console_message;
On 2013/07/18 22:15:49, Mark Seaborn wrote:
> Shouldn't there be some punctuation between the messages, like a colon? 
What's
> an example of the output here?  Isn't this going to output something like
> "ServiceRuntime: failed to startNaClSocketPair() failed"?

I used semicolon so that we don't have 2 colons in a row.

[Mark Seaborn, 2013-07-19 15:25:30]

LGTM

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://chromiumcodereview.appspot.com/18045007/diff/200001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:129: console_message_ =
message + console_message;
On 2013/07/19 09:06:20, halyavin wrote:
> On 2013/07/18 22:15:49, Mark Seaborn wrote:
> > Shouldn't there be some punctuation between the messages, like a colon? 
> > What's an example of the output here?  Isn't this going to output
> > something like "ServiceRuntime: failed to startNaClSocketPair() failed"?
> 
> I used semicolon so that we don't have 2 colons in a row.

FWIW, I think multiple colons in a sentence are OK.

https://chromiumcodereview.appspot.com/18045007/diff/221001/ppapi/native_clie...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://chromiumcodereview.appspot.com/18045007/diff/221001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:121: // console_message is
a part of error that is logged to JavaScript console
Nit: "the error"; "the JavaScript console"

https://chromiumcodereview.appspot.com/18045007/diff/221001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:122: // but is not
reported to JavaScript via lastError property.  This is used
"the lastError property"

https://chromiumcodereview.appspot.com/18045007/diff/221001/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin_error.h:123: // to report internal
errors which may easily change in new versions of
This doesn't really explain that we're hiding the message intentionally.  Maybe
add "and we don't want apps to come to depend on the details of these errors".

[halyavin, 2013-07-22 09:11:35]

https://codereview.chromium.org/18045007/diff/221001/ppapi/native_client/src/...
File ppapi/native_client/src/trusted/plugin/plugin_error.h (right):

https://codereview.chromium.org/18045007/diff/221001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin_error.h:121: // console_message is
a part of error that is logged to JavaScript console
On 2013/07/19 15:25:30, Mark Seaborn wrote:
> Nit: "the error"; "the JavaScript console"

Done.

https://codereview.chromium.org/18045007/diff/221001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin_error.h:122: // but is not
reported to JavaScript via lastError property.  This is used
On 2013/07/19 15:25:30, Mark Seaborn wrote:
> "the lastError property"

Done.

https://codereview.chromium.org/18045007/diff/221001/ppapi/native_client/src/...
ppapi/native_client/src/trusted/plugin/plugin_error.h:123: // to report internal
errors which may easily change in new versions of
On 2013/07/19 15:25:30, Mark Seaborn wrote:
> This doesn't really explain that we're hiding the message intentionally. 
Maybe
> add "and we don't want apps to come to depend on the details of these errors".

Done.

[commit-bot: I haz the power, 2013-07-22 15:45:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/18045007/260001

[commit-bot: I haz the power, 2013-07-22 15:52:36]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-22 16:56:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/18045007/260001

[commit-bot: I haz the power, 2013-07-22 18:01:32]

Change committed as 212913