[d8] console methods must not throw.

[d8] console methods must not throw.

R=jarin@chromium.org
BUG=chromium:714696
Review-Url: https://codereview.chromium.org/2838143002
Cr-Original-Commit-Position: refs/heads/master@{#44854}
Committed: https://chromium.googlesource.com/v8/v8/+/87b5b53f6f3321ad33b15e686590da7b57df2ff9
Review-Url: https://codereview.chromium.org/2838143002
Cr-Commit-Position: refs/heads/master@{#44880}
Committed: https://chromium.googlesource.com/v8/v8/+/a6b27a725fd233fdbd52fc7519b32deab0c0e05a

[Yang, 2017-04-25 10:00:11]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 10:00:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 10:26:34]

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

[commit-bot: I haz the power, 2017-04-25 10:26:35]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-04-25 13:45:10]

lgtm

[Yang, 2017-04-25 13:46:15]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2017-04-25 13:46:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 13:47:36]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493127975707460, "parent_rev":
"fed3e09356004521023253cfea903e72c42c2e82", "commit_rev":
"87b5b53f6f3321ad33b15e686590da7b57df2ff9"}

[commit-bot: I haz the power, 2017-04-25 13:47:40]

Description was changed from

==========
[d8] console methods must not throw.

R=jarin@chromium.org
BUG=chromium:714696
==========

to

==========
[d8] console methods must not throw.

R=jarin@chromium.org
BUG=chromium:714696

Review-Url: https://codereview.chromium.org/2838143002
Cr-Commit-Position: refs/heads/master@{#44854}
Committed:
https://chromium.googlesource.com/v8/v8/+/87b5b53f6f3321ad33b15e686590da7b57d...
==========

[commit-bot: I haz the power, 2017-04-25 13:47:41]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/v8/v8/+/87b5b53f6f3321ad33b15e686590da7b57d...

[Yang, 2017-04-25 14:00:45]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2840853002/ by yangguo@chromium.org.

The reason for reverting is: Breaks no-intl builds..

[Yang, 2017-04-25 19:43:30]

Description was changed from

==========
[d8] console methods must not throw.

R=jarin@chromium.org
BUG=chromium:714696

Review-Url: https://codereview.chromium.org/2838143002
Cr-Commit-Position: refs/heads/master@{#44854}
Committed:
https://chromium.googlesource.com/v8/v8/+/87b5b53f6f3321ad33b15e686590da7b57d...
==========

to

==========
[d8] console methods must not throw.

R=jarin@chromium.org
BUG=chromium:714696

Review-Url: https://codereview.chromium.org/2838143002
Cr-Commit-Position: refs/heads/master@{#44854}
Committed:
https://chromium.googlesource.com/v8/v8/+/87b5b53f6f3321ad33b15e686590da7b57d...
==========

[Yang, 2017-04-26 09:25:09]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2017-04-26 09:25:10]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2838143002/#ps20001
(title: "fix for non-intl build")

[commit-bot: I haz the power, 2017-04-26 09:25:21]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 09:48:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493198709576710,
"parent_rev": "2c460f6a267b21e80e27e9c5624b5670a6c4e1db", "commit_rev":
"a6b27a725fd233fdbd52fc7519b32deab0c0e05a"}

[commit-bot: I haz the power, 2017-04-26 09:48:35]

Description was changed from

==========
[d8] console methods must not throw.

R=jarin@chromium.org
BUG=chromium:714696

Review-Url: https://codereview.chromium.org/2838143002
Cr-Commit-Position: refs/heads/master@{#44854}
Committed:
https://chromium.googlesource.com/v8/v8/+/87b5b53f6f3321ad33b15e686590da7b57d...
==========

to

==========
[d8] console methods must not throw.

R=jarin@chromium.org
BUG=chromium:714696

Review-Url: https://codereview.chromium.org/2838143002
Cr-Original-Commit-Position: refs/heads/master@{#44854}
Committed:
https://chromium.googlesource.com/v8/v8/+/87b5b53f6f3321ad33b15e686590da7b57d...
Review-Url: https://codereview.chromium.org/2838143002
Cr-Commit-Position: refs/heads/master@{#44880}
Committed:
https://chromium.googlesource.com/v8/v8/+/a6b27a725fd233fdbd52fc7519b32deab0c...
==========

[commit-bot: I haz the power, 2017-04-26 09:48:36]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/a6b27a725fd233fdbd52fc7519b32deab0c...

[kozy, 2017-04-26 23:00:41]

kozyatinskiy@chromium.org changed reviewers:
+ kozyatinskiy@chromium.org

[kozy, 2017-04-26 23:00:42]

https://codereview.chromium.org/2838143002/diff/20001/src/builtins/builtins-c...
File src/builtins/builtins-console.cc (right):

https://codereview.chromium.org/2838143002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-console.cc:48: CHECK(!isolate->has_pending_exception());  
\
What is motivation behind this check?
I just wondering because there is some current console-spec work in progress and
one of the proposals is to use real toString [1] for
console.time({toString:()=>"123"}) and in some other methods.

[1] crbug.com/696805

[Yang, 2017-04-27 08:29:16]

On 2017/04/26 23:00:42, kozy wrote:
>
https://codereview.chromium.org/2838143002/diff/20001/src/builtins/builtins-c...
> File src/builtins/builtins-console.cc (right):
> 
>
https://codereview.chromium.org/2838143002/diff/20001/src/builtins/builtins-c...
> src/builtins/builtins-console.cc:48: CHECK(!isolate->has_pending_exception());
 
> \
> What is motivation behind this check?
> I just wondering because there is some current console-spec work in progress
and
> one of the proposals is to use real toString [1] for
> console.time({toString:()=>"123"}) and in some other methods.
> 
> [1] crbug.com/696805

Console builtins currently do not propagate exceptions. If the console builtins
triggers an exception and does not catch it, the isolate will have a pending
exception.
From how the console builtins behave in Chrome, they are expected to swallow
exceptions. Which is what is done here as well. In the d8 implementation, we do
call toString. But if that throws, the exception is caught and discarded. The
CHECK simply guards this requirement.

[kozy, 2017-04-27 18:46:27]

On 2017/04/27 08:29:16, Yang wrote:
> On 2017/04/26 23:00:42, kozy wrote:
> >
>
https://codereview.chromium.org/2838143002/diff/20001/src/builtins/builtins-c...
> > File src/builtins/builtins-console.cc (right):
> > 
> >
>
https://codereview.chromium.org/2838143002/diff/20001/src/builtins/builtins-c...
> > src/builtins/builtins-console.cc:48:
CHECK(!isolate->has_pending_exception());
>  
> > \
> > What is motivation behind this check?
> > I just wondering because there is some current console-spec work in progress
> and
> > one of the proposals is to use real toString [1] for
> > console.time({toString:()=>"123"}) and in some other methods.
> > 
> > [1] crbug.com/696805
> 
> Console builtins currently do not propagate exceptions. If the console
builtins
> triggers an exception and does not catch it, the isolate will have a pending
> exception.
> From how the console builtins behave in Chrome, they are expected to swallow
> exceptions. Which is what is done here as well. In the d8 implementation, we
do
> call toString. But if that throws, the exception is caught and discarded. The
> CHECK simply guards this requirement.

I see, thank you!