Throw JavaScript error instead of CHECK when this in privates is invalid

Throw JavaScript error instead of CHECK when this is invalid.

The extension system frequently uses privates(this).
this can easily be set to a non-object or a null value by accident,
so throw an exception instead of terminating the renderer.

Example:
var addListener = chrome.runtime.onMessage.addListener;
addListener(function() { });
// In strict mode, the "this" of addListener is undefined, which
// causes the CHECK to be triggered and the renderer to be killed:
module_system.cc(563)] Check failed: args[0]->IsObject()

R=kalman@chromium.org
TEST=manually; ran Chrome, installed any extension and pasted the
     previous code snippet in the console of the extension, and
     observed that a JavaScript error was thrown (in the console).

Committed: https://crrev.com/5c641eddb3b30f6fe62c972ae4bfcb88e8985714
Cr-Commit-Position: refs/heads/master@{#307385}

[robwu, 2014-12-08 22:17:32]

Bug report:
https://groups.google.com/a/chromium.org/d/msg/chromium-extensions/05jW7aap1s...

[not at google - send to devlin, 2014-12-08 22:29:11]

https://codereview.chromium.org/783333002/diff/1/extensions/renderer/module_s...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/783333002/diff/1/extensions/renderer/module_s...
extensions/renderer/module_system.cc:570: return;
Good catch, I think this is the right behaviour, though
- should throw a TypeError
- the explanation could be better and differentiate between wrong type and
undefined type. The former is something dumb, the latter is something subtle
that every JS programmer makes at some point.

So I'd say like:

if (args[0]->IsUndefined()) {
  GetIsolate()->ThrowException(v8::Exception::TypeError(
      "Method called without a receiver. Did you forget to call .bind()?");
}
... etc.

[robwu, 2014-12-08 22:39:24]

https://codereview.chromium.org/783333002/diff/1/extensions/renderer/module_s...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/783333002/diff/1/extensions/renderer/module_s...
extensions/renderer/module_system.cc:570: return;
On 2014/12/08 22:29:11, kalman wrote:
> Good catch, I think this is the right behaviour, though
> - should throw a TypeError
> - the explanation could be better and differentiate between wrong type and
> undefined type. The former is something dumb, the latter is something subtle
> that every JS programmer makes at some point.
> 
> So I'd say like:
> 
> if (args[0]->IsUndefined()) {
>   GetIsolate()->ThrowException(v8::Exception::TypeError(
>       "Method called without a receiver. Did you forget to call .bind()?");
> }
> ... etc.

Done.

[not at google - send to devlin, 2014-12-08 22:48:14]

lgtm

https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:568: args[0]->IsUndefined() ?
I think the ternary here looks pretty weird. I had to read every line of code to
figure out what happened. IMO separating out into 2 cases, the if-undefined
case, and the else-if-otherwise-invalid case, would be better. However, I don't
feel feel that strongly.

[robwu, 2014-12-08 22:51:28]

https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:568: args[0]->IsUndefined() ?
On 2014/12/08 22:48:11, kalman wrote:
> I think the ternary here looks pretty weird. I had to read every line of code
to
> figure out what happened. IMO separating out into 2 cases, the if-undefined
> case, and the else-if-otherwise-invalid case, would be better. However, I
don't
> feel feel that strongly.

Would it help if I put the ? and : at the start of the line? I used a ternary
because I wanted to emphasize that this branch throws an error regardless of the
message.

[not at google - send to devlin, 2014-12-08 22:59:38]

On 2014/12/08 22:51:28, robwu wrote:
>
https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
> File extensions/renderer/module_system.cc (right):
> 
>
https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
> extensions/renderer/module_system.cc:568: args[0]->IsUndefined() ?
> On 2014/12/08 22:48:11, kalman wrote:
> > I think the ternary here looks pretty weird. I had to read every line of
code
> to
> > figure out what happened. IMO separating out into 2 cases, the if-undefined
> > case, and the else-if-otherwise-invalid case, would be better. However, I
> don't
> > feel feel that strongly.
> 
> Would it help if I put the ? and : at the start of the line? I used a ternary
> because I wanted to emphasize that this branch throws an error regardless of
the
> message.

Do whatever clang-format decides to do. Something like that would help, though.
It looks like a continual string block when really it's not.

[robwu, 2014-12-08 23:08:38]

https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/783333002/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system.cc:568: args[0]->IsUndefined() ?
On 2014/12/08 22:51:27, robwu wrote:
> On 2014/12/08 22:48:11, kalman wrote:
> > I think the ternary here looks pretty weird. I had to read every line of
code
> to
> > figure out what happened. IMO separating out into 2 cases, the if-undefined
> > case, and the else-if-otherwise-invalid case, would be better. However, I
> don't
> > feel feel that strongly.
> 
> Would it help if I put the ? and : at the start of the line? I used a ternary
> because I wanted to emphasize that this branch throws an error regardless of
the
> message.

Done (I put ? and : in front of the line and ran git cl format extensions).

[not at google - send to devlin, 2014-12-08 23:18:32]

lgtm

https://codereview.chromium.org/783333002/diff/40001/extensions/renderer/modu...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/783333002/diff/40001/extensions/renderer/modu...
extensions/renderer/module_system.cc:572: : "Invalid invocation: this is not an
object!")));
s/this/receiver.

[robwu, 2014-12-08 23:35:08]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2014-12-08 23:35:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/783333002/60001

[commit-bot: I haz the power, 2014-12-09 00:25:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-09 00:25:49]

Patchset 4 (id:??) landed as
https://crrev.com/5c641eddb3b30f6fe62c972ae4bfcb88e8985714
Cr-Commit-Position: refs/heads/master@{#307385}