Use console.log to report NaCl errors in Chrome instead of alerts

Use console.log to report NaCl errors in Chrome instead of alerts
BUG=51598
Committed: http://code.google.com/p/nativeclient/source/detail?r=3067

[Mark Seaborn, 2010-08-18 20:08:08]

Can you disambiguate the bug number by using a full URL, please?

[gregoryd, 2010-08-18 20:12:04]

On 2010/08/18 20:08:08, Mark Seaborn wrote:
> Can you disambiguate the bug number by using a full URL, please?

Done (though the reviews site usually generates links to Chrome bugs
automatically)

[Mark Seaborn, 2010-08-18 20:18:59]

http://codereview.chromium.org/3119028/diff/8001/9002
File src/trusted/plugin/npapi/browser_impl_npapi.cc (right):

http://codereview.chromium.org/3119028/diff/8001/9002#newcode123
src/trusted/plugin/npapi/browser_impl_npapi.cc:123: nacl::string
eval_command("console.log(\"");
It's not good to use eval()...  See how Alert() uses npruntime to do this using
a proper object invocation.

[gregoryd, 2010-08-18 22:27:14]

Please take another look

[Mark Seaborn, 2010-08-18 22:41:01]

Quick comments because it's late...

http://codereview.chromium.org/3119028/diff/16001/17002
File src/trusted/plugin/npapi/browser_impl_npapi.cc (right):

http://codereview.chromium.org/3119028/diff/16001/17002#newcode118
src/trusted/plugin/npapi/browser_impl_npapi.cc:118: return Alert(instance_id,
msg);
msg -> text

http://codereview.chromium.org/3119028/diff/16001/17002#newcode134
src/trusted/plugin/npapi/browser_impl_npapi.cc:134: return false;
I think you need to do NPN_ReleaseObject(window) on the error path here, and
later too.  But check the NPAPI docs.

http://codereview.chromium.org/3119028/diff/16001/17002#newcode136
src/trusted/plugin/npapi/browser_impl_npapi.cc:136: NPObject* console_object =
NPVARIANT_TO_OBJECT(console_variant);
Need to check that NPVARIANT_IS_OBJECT(console_variant).  Especially because
window.console is under the control of untrusted Javascript.

[gregoryd, 2010-08-19 00:06:43]

+polina - added PPAPI implementation.

http://codereview.chromium.org/3119028/diff/16001/17002
File src/trusted/plugin/npapi/browser_impl_npapi.cc (right):

http://codereview.chromium.org/3119028/diff/16001/17002#newcode118
src/trusted/plugin/npapi/browser_impl_npapi.cc:118: return Alert(instance_id,
msg);
On 2010/08/18 22:41:01, Mark Seaborn wrote:
> msg -> text

Done.

http://codereview.chromium.org/3119028/diff/16001/17002#newcode134
src/trusted/plugin/npapi/browser_impl_npapi.cc:134: return false;
On 2010/08/18 22:41:01, Mark Seaborn wrote:
> I think you need to do NPN_ReleaseObject(window) on the error path here, and
> later too.  But check the NPAPI docs.

Done.

http://codereview.chromium.org/3119028/diff/16001/17002#newcode136
src/trusted/plugin/npapi/browser_impl_npapi.cc:136: NPObject* console_object =
NPVARIANT_TO_OBJECT(console_variant);
On 2010/08/18 22:41:01, Mark Seaborn wrote:
> Need to check that NPVARIANT_IS_OBJECT(console_variant).  Especially because
> window.console is under the control of untrusted Javascript.

Done.

[sehr (please use chromium), 2010-08-19 00:10:13]

One suggestion regarding removing a label.  Otherwise, LGTM.

http://codereview.chromium.org/3119028/diff/29006/34002
File src/trusted/plugin/npapi/browser_impl_npapi.cc (right):

http://codereview.chromium.org/3119028/diff/29006/34002#newcode154
src/trusted/plugin/npapi/browser_impl_npapi.cc:154:
NPN_ReleaseVariantValue(&console_variant);
If you use

if (!NPVARIANT_IS_VOID(console_variant)) around this and initialize it at the
beginning, you can just have one label and always goto that.

[gregoryd, 2010-08-19 00:17:38]

http://codereview.chromium.org/3119028/diff/29006/34002
File src/trusted/plugin/npapi/browser_impl_npapi.cc (right):

http://codereview.chromium.org/3119028/diff/29006/34002#newcode154
src/trusted/plugin/npapi/browser_impl_npapi.cc:154:
NPN_ReleaseVariantValue(&console_variant);
On 2010/08/19 00:10:13, sehr wrote:
> If you use
> 
> if (!NPVARIANT_IS_VOID(console_variant)) around this and initialize it at the
> beginning, you can just have one label and always goto that. 

I thought about it and decided not to do it: can I be sure that NPN_GetProperty
does not mess with console_variant when it fails?

[polina, 2010-08-19 00:48:46]

http://codereview.chromium.org/3119028/diff/29006/34005
File src/trusted/plugin/ppapi/browser_interface_ppapi.cc (right):

http://codereview.chromium.org/3119028/diff/29006/34005#newcode77
src/trusted/plugin/ppapi/browser_interface_ppapi.cc:77: console.Call("log",
text, &exception);
actually we can just do:

instance->GetWindowObject().GetProperty("console", ...).Call("log"....);
return exception.is_void()

I think GetWindow should just return pp::Var.

[gregoryd, 2010-08-19 01:05:42]

http://codereview.chromium.org/3119028/diff/29006/34005
File src/trusted/plugin/ppapi/browser_interface_ppapi.cc (right):

http://codereview.chromium.org/3119028/diff/29006/34005#newcode77
src/trusted/plugin/ppapi/browser_interface_ppapi.cc:77: console.Call("log",
text, &exception);
On 2010/08/19 00:48:46, polina wrote:
> actually we can just do:
> 
> instance->GetWindowObject().GetProperty("console", ...).Call("log"....);
> return exception.is_void()
> 
> I think GetWindow should just return pp::Var.
> 

Done.

[polina, 2010-08-19 01:24:57]

LGTM for ppapi after you fix the comment and test this manually using
examples_ppapi.html > SRPC resource descriptor transfer.

Thanks, 
P.

http://codereview.chromium.org/3119028/diff/44001/27007
File src/trusted/plugin/ppapi/browser_interface_ppapi.h (right):

http://codereview.chromium.org/3119028/diff/44001/27007#newcode48
src/trusted/plugin/ppapi/browser_interface_ppapi.h:48: // Write to the debugging
console.
s/debugging/JavaScript/

[gregoryd, 2010-08-19 21:02:14]

On 2010/08/19 01:24:57, polina wrote:
> LGTM for ppapi after you fix the comment and test this manually using
> examples_ppapi.html > SRPC resource descriptor transfer.
> 
> Thanks, 
> P.
> 
> http://codereview.chromium.org/3119028/diff/44001/27007
> File src/trusted/plugin/ppapi/browser_interface_ppapi.h (right):
> 
> http://codereview.chromium.org/3119028/diff/44001/27007#newcode48
> src/trusted/plugin/ppapi/browser_interface_ppapi.h:48: // Write to the
debugging
> console.
> s/debugging/JavaScript/

Done

[polina, 2010-08-19 22:09:05]

LGTM for ppapi
thanks for running the tests

[gregoryd, 2010-08-20 18:30:52]

On 2010/08/19 22:09:05, polina wrote:
> LGTM for ppapi
> thanks for running the tests

David and Mark: do you have any additional comments?

[Mark Seaborn, 2010-08-20 18:45:32]

On 2010/08/20 18:30:52, gregoryd wrote:
> On 2010/08/19 22:09:05, polina wrote:
> > LGTM for ppapi
> > thanks for running the tests
> 
> David and Mark: do you have any additional comments?

You addressed my comments, so thanks and LGTM!

http://codereview.chromium.org/3119028/diff/42002/48003
File src/trusted/plugin/npapi/browser_impl_npapi.cc (right):

http://codereview.chromium.org/3119028/diff/42002/48003#newcode131
src/trusted/plugin/npapi/browser_impl_npapi.cc:131:
VOID_TO_NPVARIANT(console_variant);
If you're using gotos for cleanup you don't need this line.

[Cliff L. Biffle, 2010-08-23 18:11:38]

This CL has broken Chrome integration at r3067 -- it doesn't compile with -Werr
on gcc.  I'm actually confused as to why it didn't break the NaCl builds as
well.  gregoryd is fixing.

[Cliff L. Biffle, 2010-08-23 18:12:27]

Ah: this did not break the NaCl builds because the offending code is within
#ifndef NACL_STANDALONE, and we only test NACL_STANDALONE.  This is arguably a
bug in our build.  Filing an issue.

[polina, 2010-08-26 02:45:15]

http://codereview.chromium.org/3119028/diff/42002/48005
File src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/3119028/diff/42002/48005#newcode620
src/trusted/plugin/plugin.cc:620: browser_interface_->Alert(instance_id(),
error_string);
This is still an Alert. Is that on purpose?