Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(68)

Issue 6397011: Make exception thrown via v8 public API propagate to v8::TryCatch as JS thrown exceptions do. (Closed)

Created:
9 years, 8 months ago by antonm
Modified:
9 years, 1 month ago
CC:
v8-dev
Visibility:
Public.

Description

Make exception thrown via v8 public API propagate to v8::TryCatch as JS thrown exceptions do. Correctly process failures which can be returned by Object::GetProperty when performing GetRealNamedProperty* queries. Callback properties can produce exceptions so we need to wrap access to them into exception checks. However, despite of many other methods with exception checks, property access doesn't mandatroy go via JavaScript and hence we need to inject code to propagate exception to public API TryCatch handlers. Committed: http://code.google.com/p/v8/source/detail?r=7258

Patch Set 1 #

Total comments: 28

Patch Set 2 : Addressing Mads' concerns #

Patch Set 3 : Addressing Vitaly's comments #

Total comments: 10

Patch Set 4 : Addressing Vitaly's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+269 lines, -54 lines) Patch
M src/api.cc View 1 2 3 4 chunks +24 lines, -22 lines 0 comments Download
M src/handles.h View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M src/handles.cc View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
M src/messages.cc View 1 2 3 2 chunks +15 lines, -0 lines 0 comments Download
M src/top.h View 1 2 3 4 chunks +50 lines, -11 lines 0 comments Download
M src/top.cc View 1 2 3 chunks +33 lines, -21 lines 0 comments Download
M test/cctest/test-api.cc View 1 2 3 3 chunks +133 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
antonm
Mads and Vitaly, may you have a look? This patch requires some adjustments in V8 ...
9 years, 8 months ago (2011-01-28 10:10:28 UTC) #1
Mads Ager (chromium)
http://codereview.chromium.org/6397011/diff/1/src/api.cc File src/api.cc (right): http://codereview.chromium.org/6397011/diff/1/src/api.cc#newcode2585 src/api.cc:2585: has_pending_exception = value->IsFailure(); Should you test for value->IsException instead ...
9 years, 8 months ago (2011-01-28 11:39:18 UTC) #2
antonm
http://codereview.chromium.org/6397011/diff/1/src/api.cc File src/api.cc (right): http://codereview.chromium.org/6397011/diff/1/src/api.cc#newcode2585 src/api.cc:2585: has_pending_exception = value->IsFailure(); On 2011/01/28 11:39:18, Mads Ager wrote: ...
9 years, 8 months ago (2011-01-28 13:37:24 UTC) #3
Vitaly Repeshko
Thanks for looking into this stuff! I have a bunch of questions for now and ...
9 years, 8 months ago (2011-01-28 14:03:33 UTC) #4
antonm
Thanks a lot for comments, Vitaly http://codereview.chromium.org/6397011/diff/1/src/api.cc File src/api.cc (right): http://codereview.chromium.org/6397011/diff/1/src/api.cc#newcode1411 src/api.cc:1411: i::Object* exception = ...
9 years, 8 months ago (2011-01-28 14:43:18 UTC) #5
Vitaly Repeshko
LGTM with a few more comments. http://codereview.chromium.org/6397011/diff/13001/src/api.cc File src/api.cc (right): http://codereview.chromium.org/6397011/diff/13001/src/api.cc#newcode2581 src/api.cc:2581: receiver->GetProperty(*receiver, lookup, *name, ...
9 years, 8 months ago (2011-02-01 19:07:17 UTC) #6
antonm
9 years, 8 months ago (2011-02-01 20:14:39 UTC) #7
Hopefully all the comments are addressed.

I am not going to submit it right now---first I need WebKit counterpart to be
submitted.

Thanks a lot for review, Mads and Vitaly.

http://codereview.chromium.org/6397011/diff/13001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6397011/diff/13001/src/api.cc#newcode2581
src/api.cc:2581: receiver->GetProperty(*receiver, lookup, *name, &attributes);
On 2011/02/01 19:07:17, Vitaly wrote:
> Wrap this in CALL_HEAP_FUNCTION?

Done.

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc
File src/messages.cc (right):

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc#newcode134
src/messages.cc:134: Top::Scope top_scope;
On 2011/02/01 19:07:17, Vitaly wrote:
> I don't like the name. ExceptionScope?
> Also given that we explicitly manipulate some of its fields here, maybe we
> should just inline this object into this function.

Renamed to ExceptionScope.

http://codereview.chromium.org/6397011/diff/13001/src/messages.cc#newcode145
src/messages.cc:145: // Consider logging it somehow.
On 2011/02/01 19:07:17, Vitaly wrote:
> Please file a bug to not forget about "scary" exceptions here.

Will do.

http://codereview.chromium.org/6397011/diff/13001/src/top.h
File src/top.h (right):

http://codereview.chromium.org/6397011/diff/13001/src/top.h#newcode539
src/top.h:539: Scope() :
On 2011/02/01 19:07:17, Vitaly wrote:
> Does it lint?

Yes

http://codereview.chromium.org/6397011/diff/13001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6397011/diff/13001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:8016: CHECK(!withJsGetter.IsEmpty());
On 2011/02/01 19:07:17, Vitaly wrote:
> nit: Avoid camel case.

Done.

Powered by Google App Engine
This is Rietveld 408576698