Cleanly handle excessively long JS source strings.

Cleanly handle excessively long JS source strings.

BUG=463672
R=jochen@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192086

[vogelheim, 2015-03-06 17:07:52]

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:366:
V8ThrowException::throwGeneralError(isolate, "JavaScript source exceeds maximum
string length.");
Does this even make any sense?

When I reproduce the test case in the browser it will now cleanly abort
execution instead of crashing the tab. However, merely returning the empty
handle below does that, too. I've been looking for a user-visible indication of
the error, such as a console message or exception log or whatnot, so that the
web developer has a chance of knowing what's going on, but I can't find any of
it in devtools, so I wonder if I'm doing it wrong.

The way I read ScriptController::executeScriptAndReturnValue, it purposely
ignores whatever the v8::TryCatch catches, which I guess is why I'm not seeing
anything.

So... should I just remove the throwGeneralError as it's a no-op in this case;
or keep it in for the sake of other code paths? Or... something else?

[jochen (gone - plz use gerrit), 2015-03-06 17:15:22]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org, marja@chromium.org

[jochen (gone - plz use gerrit), 2015-03-06 17:15:22]

hum, no idea

maybe Marja or Kentaro know?

[marja, 2015-03-06 18:10:10]

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:366:
V8ThrowException::throwGeneralError(isolate, "JavaScript source exceeds maximum
string length.");
Hmm, there is some mechanism so that we get a console message when there is a
compile error, and I think this should just do the same. I don't know by heart
where that message comes from.

[haraken, 2015-03-07 04:17:15]

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:366:
V8ThrowException::throwGeneralError(isolate, "JavaScript source exceeds maximum
string length.");
On 2015/03/06 18:10:10, marja wrote:
> Hmm, there is some mechanism so that we get a console message when there is a
> compile error, and I think this should just do the same. I don't know by heart
> where that message comes from.

A short answer is that I don't think this check makes much sense. I think we can
remove it.

If I'm understanding correctly, V8 does not support a string larger than 200 MB
(right?). So, in the first place, you cannot pass a string larger than 200 MB
from V8 to Blink. You'll hit a crash in V8 before falling into this code path.
Thus the only case we hit the code path is that Blink tries to compile a string
larger than 256 MB. However, in this case, there is no way to see the error in
console, so throwGeneralError doesn't do anything meaningful. When Blink passes
a string larger than 256 MB, we can just let V8 crash in the following
compileScript.

In summary, I think we can remove the check.

[vogelheim, 2015-03-09 17:36:52]

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:366:
V8ThrowException::throwGeneralError(isolate, "JavaScript source exceeds maximum
string length.");
On 2015/03/06 18:10:10, marja wrote:
> Hmm, there is some mechanism so that we get a console message when there is a
> compile error, and I think this should just do the same. I don't know by heart
> where that message comes from.

Good plan, and with a detour it led to a solution:
- A C++ function that calls V8ThrowException::throwRangeError (or whatever error
we pick)
- compileScript creating a v8::Function based on the above and calls it.

It's just a few lines of code, but it doesn't make me feel pretty. :-|

https://codereview.chromium.org/989573002/diff/1/Source/bindings/core/v8/V8Sc...
Source/bindings/core/v8/V8ScriptRunner.cpp:366:
V8ThrowException::throwGeneralError(isolate, "JavaScript source exceeds maximum
string length.");
On 2015/03/07 04:17:15, haraken wrote:
> On 2015/03/06 18:10:10, marja wrote:
> > Hmm, there is some mechanism so that we get a console message when there is
a
> > compile error, and I think this should just do the same. I don't know by
heart
> > where that message comes from.
> 
> A short answer is that I don't think this check makes much sense. I think we
can
> remove it.
> 
> If I'm understanding correctly, V8 does not support a string larger than 200
MB
> (right?). So, in the first place, you cannot pass a string larger than 200 MB
> from V8 to Blink. You'll hit a crash in V8 before falling into this code path.
> Thus the only case we hit the code path is that Blink tries to compile a
string
> larger than 256 MB. However, in this case, there is no way to see the error in
> console, so throwGeneralError doesn't do anything meaningful. When Blink
passes
> a string larger than 256 MB, we can just let V8 crash in the following
> compileScript.
> 
> In summary, I think we can remove the check.

I disagree (on that part), and/or there's a misunderstanding.

Presently it crashes in compileScript, as you suggest, but that means we keep
getting crash reports when someone actually includes gigantic JS files. Which
people do. IMHO, Jakob was very much right to classify this as a proper bug and
tossing it over to me. 

We shouldn't really crash the tab based on perfectly reproducable user
behaviour. The question that's open - if you can agree with me on the above - is
how to fix it. The current CL will silently drop the script, with no hint to the
JS developer what's going wrong. Based on Marja's comment, I'll upload a new
version with a somewhat hack-ish code that will output a console message.

[vogelheim, 2015-03-09 18:33:11]

The new version throws the exception, similar to
throwStackOverflowExceptionIfNeeded earlier in the same file.

[marja, 2015-03-09 18:50:15]

I assume you tried out that this gives the console message?

[marja, 2015-03-09 18:50:20]

I assume you tried out that this gives the console message?

[vogelheim, 2015-03-09 19:16:01]

Am 09.03.2015 7:50 nachm. schrieb <marja@chromium.org>:
>
> I assume you tried out that this gives the console message?

I did, and it does. Will be happy to demonstrate.

>
> https://codereview.chromium.org/989573002/

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

[haraken, 2015-03-09 23:21:56]

https://codereview.chromium.org/989573002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/989573002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8ScriptRunner.cpp:373: v8::Function::New(isolate,
throwSourceTooLargeException)->Call(v8::Undefined(isolate), 0, 0);

Can we use a (already existing) helper function in V8ThrowException.h? We want
to centralize code that throws exceptions into V8ThrowException.h.

[haraken, 2015-03-11 09:10:26]

Continuation of our discussion offline:

- ScriptController::executeScriptAndReturnValue doesn't rethrow a TryCatch,
because it's possible that ScriptController::executeScriptAndReturnValue can be
called without having V8 above the stack.

- Thus V8ThrowException::throwException is useless inside
ScriptController::executeScriptAndReturnValue.

- So I think you can use addConsoleMessage to solve your problem.

[jochen (gone - plz use gerrit), 2015-03-12 15:02:56]

i think we need something similar to the code in
WorkerScriptController::evaluate

just passing the exception to the console is not enough, it should also fire an
onerror event

[blink-reviews, 2015-03-12 15:12:34]

Right. So my current plan is to keep trying to make the V8 exception work.
As Jochen mentions, this should ensure that this compile time error is
handled like other compile time errors. If I can't get that to work, then
I'll make the bindings addConsoleMessage for this case. I'll report back.

On Thu, Mar 12, 2015 at 4:02 PM, <jochen@chromium.org> wrote:

> i think we need something similar to the code in
> WorkerScriptController::evaluate
>
> just passing the exception to the console is not enough, it should also
> fire an
> onerror event
>
> https://codereview.chromium.org/989573002/
>

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

[haraken, 2015-03-12 23:35:42]

On 2015/03/12 15:12:34, blink-reviews wrote:
> Right. So my current plan is to keep trying to make the V8 exception work.
> As Jochen mentions, this should ensure that this compile time error is
> handled like other compile time errors. If I can't get that to work, then
> I'll make the bindings addConsoleMessage for this case. I'll report back.
> 
> On Thu, Mar 12, 2015 at 4:02 PM, <mailto:jochen@chromium.org> wrote:
> 
> > i think we need something similar to the code in
> > WorkerScriptController::evaluate
> >
> > just passing the exception to the console is not enough, it should also
> > fire an
> > onerror event
> >
> > https://codereview.chromium.org/989573002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

Sounds like a nice plan.

[vogelheim, 2015-03-16 14:44:32]

Update:
- Got a version that works, by making ScriptController's TryCatch report
exceptions.
- This reports 'true' syntax errors twice, 1x in v8/src/compiler.cc and 1x
in ScriptController.
- Jochen recommended [offline] to look at how WorkerScriptController does
it.
- It turns out that WorkerScriptController does it by also crashing the
tab, so I need to fix that path, too.
- With that fixed, WorkerScriptController works like my ScriptController
version, with 'true' syntax errors reported twice.
- If I drop reportExceptions from v8/src/compiler.cc, it seems everything
works.
- This uglifies the bindings code a bit, since then there's some
duplication between the error handling+reporting of the two
*ScriptControllers. I'll see whether I can cleanly extract the joint
functionality.



On Fri, Mar 13, 2015 at 12:35 AM, <haraken@chromium.org> wrote:

> On 2015/03/12 15:12:34, blink-reviews wrote:
>
>> Right. So my current plan is to keep trying to make the V8 exception work.
>> As Jochen mentions, this should ensure that this compile time error is
>> handled like other compile time errors. If I can't get that to work, then
>> I'll make the bindings addConsoleMessage for this case. I'll report back.
>>
>
>  On Thu, Mar 12, 2015 at 4:02 PM, <mailto:jochen@chromium.org> wrote:
>>
>
>  > i think we need something similar to the code in
>> > WorkerScriptController::evaluate
>> >
>> > just passing the exception to the console is not enough, it should also
>> > fire an
>> > onerror event
>> >
>> > https://codereview.chromium.org/989573002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:blink-reviews+unsubscribe@chromium.org.
>>
>
> Sounds like a nice plan.
>
>
> https://codereview.chromium.org/989573002/
>

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

[vogelheim, 2015-03-16 19:40:34]

PTAL. Both regular scripts + worker scripts now handle the error gracefully AND
write a decent message to the dev console.

Main changes:
- Pull the string conversion + associated error handling into V8ScriptRunner
- WorkerScriptController now stores the ErrorEvent
- Creating an ErrorEvent from a v8::Message is now in one place.

[haraken, 2015-03-17 00:10:05]

Thanks for being persistent on this! It looks like you need to fix test failures
though :)

[jochen (gone - plz use gerrit), 2015-03-17 10:36:40]

maybe we should do the macro dance after all to avoid having to rebaseline so
many tests twice...

[vogelheim, 2015-03-17 12:01:18]

Update:
- The errors were exceptions being reported twice.
  - We knew that would happen, since this moves reporting for compile time
exceptions from V8 into the bindings.
  - There's a 1 line companion change that disables this reporting in V8.
While the blink change is in, but the V8 one isn't, things might get
reported twice.
  - The "macro dance" refers to having an #ifdef to make sure both changes
are switched on simultaneously.
- It turns out that most of the error cases are runtime exceptions.
  - I fixed the code to only change behaviour for compile time exceptions
and to leave the runtime exceptions alone.
  - I'll see if this fixes all (or sufficiently many) of the test breaks.


On Tue, Mar 17, 2015 at 11:36 AM, <jochen@chromium.org> wrote:

> maybe we should do the macro dance after all to avoid having to rebaseline
> so
> many tests twice...
>
> https://codereview.chromium.org/989573002/
>

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

[blink-reviews, 2015-03-17 13:11:51]

Much smaller set of test failures now and those are indeed syntax errors
that are reported twice.

On Tue, Mar 17, 2015 at 1:01 PM, Daniel Vogelheim <vogelheim@chromium.org>
wrote:

> Update:
> - The errors were exceptions being reported twice.
>   - We knew that would happen, since this moves reporting for compile time
> exceptions from V8 into the bindings.
>   - There's a 1 line companion change that disables this reporting in V8.
> While the blink change is in, but the V8 one isn't, things might get
> reported twice.
>   - The "macro dance" refers to having an #ifdef to make sure both changes
> are switched on simultaneously.
> - It turns out that most of the error cases are runtime exceptions.
>   - I fixed the code to only change behaviour for compile time exceptions
> and to leave the runtime exceptions alone.
>   - I'll see if this fixes all (or sufficiently many) of the test breaks.
>
>
> On Tue, Mar 17, 2015 at 11:36 AM, <jochen@chromium.org> wrote:
>
>> maybe we should do the macro dance after all to avoid having to
>> rebaseline so
>> many tests twice...
>>
>> https://codereview.chromium.org/989573002/
>>
>
>

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

[vogelheim, 2015-03-17 13:21:35]

Result:
- 16x duplicate console messages for syntax errors
- 5x messages where the actual error message is different from the expected
one.

So I think I broke something for error reporting in worker threads, which
happens to be where I did the bit of refactoring. Will fix... :-]


On Tue, Mar 17, 2015 at 2:11 PM, Daniel Vogelheim <vogelheim@google.com>
wrote:

> Much smaller set of test failures now and those are indeed syntax errors
> that are reported twice.
>
>
> On Tue, Mar 17, 2015 at 1:01 PM, Daniel Vogelheim <vogelheim@chromium.org>
> wrote:
>
>> Update:
>> - The errors were exceptions being reported twice.
>>   - We knew that would happen, since this moves reporting for compile
>> time exceptions from V8 into the bindings.
>>   - There's a 1 line companion change that disables this reporting in V8.
>> While the blink change is in, but the V8 one isn't, things might get
>> reported twice.
>>   - The "macro dance" refers to having an #ifdef to make sure both
>> changes are switched on simultaneously.
>> - It turns out that most of the error cases are runtime exceptions.
>>   - I fixed the code to only change behaviour for compile time exceptions
>> and to leave the runtime exceptions alone.
>>   - I'll see if this fixes all (or sufficiently many) of the test breaks.
>>
>>
>> On Tue, Mar 17, 2015 at 11:36 AM, <jochen@chromium.org> wrote:
>>
>>> maybe we should do the macro dance after all to avoid having to
>>> rebaseline so
>>> many tests twice...
>>>
>>> https://codereview.chromium.org/989573002/
>>>
>>
>>
>

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

[vogelheim, 2015-03-18 10:15:32]

PTAL.

mstarzigner@ suggested he's working on V8 exception reporting and in particular
he has a patch in the works that would make sure V8ThrowException::throw*Error
would always log to the console directly. This eliminates the need for much of
the code I had here, and so I threw pretty much all of it away except for the
actual bug fix.

So:
- This version only checks for the failed string conversion and throws an
exception, for both regular and worker scripts.
- This eliminates the crash(es), but will not log an error.
- As soon as the V8 patch is in, the error will be logged to the console. (Works
when I try it out locally.)
- I had most of the regressions from the previous version fixed;  I could revive
that if people disagree with this approach. But I think this is a lot simpler
overall.

[jochen (gone - plz use gerrit), 2015-03-18 10:24:50]

lgtm

[haraken, 2015-03-18 11:14:54]

LGTM

https://codereview.chromium.org/989573002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/989573002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:364:
V8ThrowException::throwSyntaxError(isolate, "Source file too large.");

Can we use throwGeneralError? (We normally use syntax errors for cases that are
speced to use syntax errors.)

https://codereview.chromium.org/989573002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:374:
V8ThrowException::throwSyntaxError(isolate, "Source file too large.");

Ditto.

[vogelheim, 2015-03-18 11:28:26]

https://codereview.chromium.org/989573002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/989573002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:364:
V8ThrowException::throwSyntaxError(isolate, "Source file too large.");
On 2015/03/18 11:14:54, haraken wrote:
> 
> Can we use throwGeneralError? (We normally use syntax errors for cases that
are
> speced to use syntax errors.)

Done.

https://codereview.chromium.org/989573002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:374:
V8ThrowException::throwSyntaxError(isolate, "Source file too large.");
On 2015/03/18 11:14:54, haraken wrote:
> 
> Ditto.

Done.

[vogelheim, 2015-03-18 11:32:39]

The CQ bit was checked by vogelheim@chromium.org

[vogelheim, 2015-03-18 11:32:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/989573002/#ps120001
(title: "syntax error => general error")

[commit-bot: I haz the power, 2015-03-18 11:32:52]

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

[commit-bot: I haz the power, 2015-03-18 12:50:29]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192086