Drop unnecessary v8::TryCatch in V8StringResource::prepare()

Skip unnecessary v8::TryCatch in V8StringResource::prepare()

Since we're only interested in detecting and propagating an exception,
check whether v8::Value::ToString()'s return value is an empty handle
instead of using a v8::TryCatch local.

This is faster, since initializing a v8::TryCatch has non-zero cost, and
should also be less binary code (v8::Handle::IsEmpty() is inlined and
simply a null check.)

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

[Jens Widell, 2014-09-19 12:10:00]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2014-09-19 12:10:01]

PTAL

[haraken, 2014-09-19 12:31:52]

LGTM

[Jens Widell, 2014-09-19 12:50:54]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-09-19 12:51:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/576323004/1

[commit-bot: I haz the power, 2014-09-19 13:54:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-19 13:54:19]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[Jens Widell, 2014-09-19 14:19:06]

On 2014/09/19 13:54:19, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

That's somewhat intresting. Two regressed tests, both of which do test failed
object-to-string conversions, so highly relevant to the change.

The fast/events/before-unload-return-bad-value.html test seems flaky (both with
and without the fix,) it performs the testing from an <iframe onload> handler
that it also triggers when testing, thus testing in an endless loop. The test
seem to behave more predictable with my fix than without, but essentially, the
flakiness (in either case) is how many times the test manages to pass (and
output various "PASS this and that" output) before its output is compared to the
expected output. I'll have a go at fixing this test. Next week.

The storage/websql/sql-error-codes.html test revealed a somewhat unexpected
difference in behavior. With the TryCatch (and ReThrow() call) the line number
in the exception message is that of the failing function call, whereas without
them the line number is that of the toString() function that threw an exception.
So it would seem that having a TryCatch and rethrowing an exception resets the
location of the error.

haraken@: Do you have a comment on the line number issue?

[haraken, 2014-09-19 14:33:19]

On 2014/09/19 14:19:06, Jens Widell wrote:
> On 2014/09/19 13:54:19, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_blink_rel on tryserver.blink
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
> 
> That's somewhat intresting. Two regressed tests, both of which do test failed
> object-to-string conversions, so highly relevant to the change.
> 
> The fast/events/before-unload-return-bad-value.html test seems flaky (both
with
> and without the fix,) it performs the testing from an <iframe onload> handler
> that it also triggers when testing, thus testing in an endless loop. The test
> seem to behave more predictable with my fix than without, but essentially, the
> flakiness (in either case) is how many times the test manages to pass (and
> output various "PASS this and that" output) before its output is compared to
the
> expected output. I'll have a go at fixing this test. Next week.
> 
> The storage/websql/sql-error-codes.html test revealed a somewhat unexpected
> difference in behavior. With the TryCatch (and ReThrow() call) the line number
> in the exception message is that of the failing function call, whereas without
> them the line number is that of the toString() function that threw an
exception.
> So it would seem that having a TryCatch and rethrowing an exception resets the
> location of the error.
> 
> haraken@: Do you have a comment on the line number issue?

I'm not sure but is it probably related to V8 bug 3514?

[Jens Widell, 2014-09-19 15:33:29]

On 2014/09/19 14:33:19, haraken(OOO til Sep 21) wrote:
> On 2014/09/19 14:19:06, Jens Widell wrote:
> > On 2014/09/19 13:54:19, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   linux_blink_rel on tryserver.blink
> > >
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
> > 
> > That's somewhat intresting. Two regressed tests, both of which do test
failed
> > object-to-string conversions, so highly relevant to the change.
> > 
> > The fast/events/before-unload-return-bad-value.html test seems flaky (both
> with
> > and without the fix,) it performs the testing from an <iframe onload>
handler
> > that it also triggers when testing, thus testing in an endless loop. The
test
> > seem to behave more predictable with my fix than without, but essentially,
the
> > flakiness (in either case) is how many times the test manages to pass (and
> > output various "PASS this and that" output) before its output is compared to
> the
> > expected output. I'll have a go at fixing this test. Next week.
> > 
> > The storage/websql/sql-error-codes.html test revealed a somewhat unexpected
> > difference in behavior. With the TryCatch (and ReThrow() call) the line
number
> > in the exception message is that of the failing function call, whereas
without
> > them the line number is that of the toString() function that threw an
> exception.
> > So it would seem that having a TryCatch and rethrowing an exception resets
the
> > location of the error.
> > 
> > haraken@: Do you have a comment on the line number issue?
> 
> I'm not sure but is it probably related to V8 bug 3514?

I don't know if it was, but here's what I think caused it:
https://codereview.chromium.org/587703002/

[Jens Widell, 2014-09-23 05:51:28]

I've updated the expected output of sql-error-codes.html; I think the old line
number was due to a V8 bug that this patch inadvertently worked around.

For now, I'm trying to ignore the before-unload-return-bad-value.html failure;
the test seems to have had problems for a while already:
http://crbug.com/398306. I tried to make the test stable by modifying it, but
failed. It seems onerror firing is unstable somehow.

[haraken, 2014-09-23 05:53:01]

On 2014/09/23 05:51:28, Jens Widell wrote:
> I've updated the expected output of sql-error-codes.html; I think the old line
> number was due to a V8 bug that this patch inadvertently worked around.
> 
> For now, I'm trying to ignore the before-unload-return-bad-value.html failure;
> the test seems to have had problems for a while already:
> http://crbug.com/398306. I tried to make the test stable by modifying it, but
> failed. It seems onerror firing is unstable somehow.

Sounds reasonable to me.

[Jens Widell, 2014-09-23 06:30:10]

On 2014/09/23 05:53:01, haraken wrote:
> On 2014/09/23 05:51:28, Jens Widell wrote:
> > I've updated the expected output of sql-error-codes.html; I think the old
line
> > number was due to a V8 bug that this patch inadvertently worked around.
> > 
> > For now, I'm trying to ignore the before-unload-return-bad-value.html
failure;
> > the test seems to have had problems for a while already:
> > http://crbug.com/398306. I tried to make the test stable by modifying it,
but
> > failed. It seems onerror firing is unstable somehow.
> 
> Sounds reasonable to me.

Ignoring before-unload-return-bad-value.html doesn't seem to work; it's
obviously failing way too reliably now.

[Jens Widell, 2014-09-23 08:30:13]

On 2014/09/23 06:30:10, Jens Widell wrote:
> On 2014/09/23 05:53:01, haraken wrote:
> > On 2014/09/23 05:51:28, Jens Widell wrote:
> > > I've updated the expected output of sql-error-codes.html; I think the old
> line
> > > number was due to a V8 bug that this patch inadvertently worked around.
> > > 
> > > For now, I'm trying to ignore the before-unload-return-bad-value.html
> failure;
> > > the test seems to have had problems for a while already:
> > > http://crbug.com/398306. I tried to make the test stable by modifying it,
> but
> > > failed. It seems onerror firing is unstable somehow.
> > 
> > Sounds reasonable to me.
> 
> Ignoring before-unload-return-bad-value.html doesn't seem to work; it's
> obviously failing way too reliably now.

Made some simple changes to the test. This appears to have stabilized it enough.

[Jens Widell, 2014-09-23 08:30:26]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-09-23 08:30:42]

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

[commit-bot: I haz the power, 2014-09-23 08:56:30]

Committed patchset #5 (id:80001) as 182476