Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(464)

Issue 559553003: In V8Binding.cpp, throw all exceptions via ExceptionState& argument (Closed)

Created:
6 years, 3 months ago by Jens Widell
Modified:
6 years, 3 months ago
Reviewers:
haraken, yhirano
CC:
blink-reviews, blink-reviews-bindings_chromium.org, arv+blink, abarth-chromium
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

In V8Binding.cpp, throw all exceptions via ExceptionState& argument In those the conversion functions that take an ExceptionState& argument, always throw via the ExceptionState object, and never directly to V8. Various existing callers assume this, and fail to detect exceptions that were thrown directly to V8. This misbehavior is a regression from https://codereview.chromium.org/313033002/ which changed the definition of the TONATIVE_DEFAULT_EXCEPTIONSTATE() macro, but these functions were somewhat tricky to use even before then, since exceptions were sometimes thrown to V8 and sometimes not, while always stored in the ExceptionState object. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181709

Patch Set 1 #

Total comments: 7

Patch Set 2 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -25 lines) Patch
M Source/bindings/core/v8/V8Binding.cpp View 1 11 chunks +58 lines, -25 lines 0 comments Download

Messages

Total messages: 13 (2 generated)
Jens Widell
PTAL I chose not to introduce yet another TONATIVE_* macro variant for this use-case, since ...
6 years, 3 months ago (2014-09-09 15:06:38 UTC) #2
Jens Widell
With this fixed, we can follow up with this: https://codereview.chromium.org/552143004
6 years, 3 months ago (2014-09-09 15:48:42 UTC) #3
haraken
LGTM https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp File Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp#newcode212 Source/bindings/core/v8/V8Binding.cpp:212: if (numberObject.IsEmpty()) { On 2014/09/09 15:06:38, Jens Widell ...
6 years, 3 months ago (2014-09-10 01:09:24 UTC) #4
Jens Widell
https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp File Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp#newcode213 Source/bindings/core/v8/V8Binding.cpp:213: exceptionState.throwTypeError("Not convertible to a number value (of type '" ...
6 years, 3 months ago (2014-09-10 04:42:27 UTC) #5
haraken
https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp File Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp#newcode213 Source/bindings/core/v8/V8Binding.cpp:213: exceptionState.throwTypeError("Not convertible to a number value (of type '" ...
6 years, 3 months ago (2014-09-10 04:49:49 UTC) #6
Jens Widell
On 2014/09/10 04:49:49, haraken wrote: > https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp > File Source/bindings/core/v8/V8Binding.cpp (right): > > https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp#newcode213 > ...
6 years, 3 months ago (2014-09-10 04:56:21 UTC) #7
Jens Widell
https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp File Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Binding.cpp#newcode212 Source/bindings/core/v8/V8Binding.cpp:212: if (numberObject.IsEmpty()) { On 2014/09/10 01:09:23, haraken wrote: > ...
6 years, 3 months ago (2014-09-10 05:33:57 UTC) #8
haraken
On 2014/09/10 04:56:21, Jens Widell wrote: > On 2014/09/10 04:49:49, haraken wrote: > > > ...
6 years, 3 months ago (2014-09-10 07:04:19 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jl@opera.com/559553003/20001
6 years, 3 months ago (2014-09-10 07:06:13 UTC) #11
commit-bot: I haz the power
Committed patchset #2 (id:20001) as 181709
6 years, 3 months ago (2014-09-10 07:09:31 UTC) #12
yhirano
6 years, 3 months ago (2014-09-10 07:19:01 UTC) #13
Message was sent while issue was closed.
Sorry for being late, lgtm

Powered by Google App Engine
This is Rietveld 408576698