Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
313033002:
Rethrow exception when swallowing it is not intended. (Closed)
|
Created:
6 years, 6 months ago by yhirano Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, arv+blink, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionRethrow exception when swallowing it is not intended.
We're trying to make v8::TryCatch nestable.
In preparation for that, this CL adds ReThrow for nested trycatches.
BUG=362388
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176411
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 11
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (left): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:92: exceptionState.rethrowV8Exception(exceptionCatcher.Exception()); In core/dom/{NodeIterator,TreeWalker}.cpp, the passed in ExceptionState object's hadException() is used to check if acceptNode() threw an exception, in which case the calling algorithm is aborted. Since you're no longer updating the ExceptionState object, but rather rethrowing the exception directly via V8's API, won't these algorithms continue, ignoring exceptions along the way?
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (left): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:92: exceptionState.rethrowV8Exception(exceptionCatcher.Exception()); On 2014/06/13 11:22:06, Jens Lindström wrote: > In core/dom/{NodeIterator,TreeWalker}.cpp, the passed in ExceptionState object's > hadException() is used to check if acceptNode() threw an exception, in which > case the calling algorithm is aborted. > > Since you're no longer updating the ExceptionState object, but rather rethrowing > the exception directly via V8's API, won't these algorithms continue, ignoring > exceptions along the way? See this TC: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3060
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (left): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:92: exceptionState.rethrowV8Exception(exceptionCatcher.Exception()); On 2014/06/13 11:22:06, Jens Lindström wrote: > In core/dom/{NodeIterator,TreeWalker}.cpp, the passed in ExceptionState object's > hadException() is used to check if acceptNode() threw an exception, in which > case the calling algorithm is aborted. > > Since you're no longer updating the ExceptionState object, but rather rethrowing > the exception directly via V8's API, won't these algorithms continue, ignoring > exceptions along the way? Thanks, done.
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (left): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:92: exceptionState.rethrowV8Exception(exceptionCatcher.Exception()); On 2014/06/16 11:30:10, yhirano wrote: > On 2014/06/13 11:22:06, Jens Lindström wrote: > > In core/dom/{NodeIterator,TreeWalker}.cpp, the passed in ExceptionState > object's > > hadException() is used to check if acceptNode() threw an exception, in which > > case the calling algorithm is aborted. > > > > Since you're no longer updating the ExceptionState object, but rather > rethrowing > > the exception directly via V8's API, won't these algorithms continue, ignoring > > exceptions along the way? > > Thanks, done. I haven't tested, but I'd guess the callers work again now. However, I think the behavior of this function is a bit odd now. I'd interpret the ExceptionState argument as "this function throws exceptions via its 'exceptionState' argument". But that's not exactly what it does. I'll leave it to the owners to say if that's a problem or not. https://codereview.chromium.org/313033002/diff/40001/Source/bindings/v8/V8Bin... File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/313033002/diff/40001/Source/bindings/v8/V8Bin... Source/bindings/v8/V8BindingMacros.h:76: exceptionState.rethrowV8Exception(block.Exception()); \ Given current usage of this macro and the one below (here, in V8ElementCustom.cpp and in generated bindings code), I'm pretty sure it's pointless to call rethrowV8Exception() here now, since 'block' will re-throw the caught exception on its own, and 'exceptionState' is a local that goes out of scope when you return. So I reckon this could be if (UNLIKELY(block.HasCaught() || exceptionState.throwIfNeeded())) return; instead. And if these macros were supposed to be useful in general code, I'd assume the user would expect the resulting exception to be in 'exceptionState' only, and not already have been rethrown via the V8 API. But I'm of course quite new to this code. :-)
https://codereview.chromium.org/313033002/diff/40001/Source/bindings/v8/V8Bin... File Source/bindings/v8/V8BindingMacros.h (right): https://codereview.chromium.org/313033002/diff/40001/Source/bindings/v8/V8Bin... Source/bindings/v8/V8BindingMacros.h:76: exceptionState.rethrowV8Exception(block.Exception()); \ On 2014/06/16 11:56:39, Jens Lindström wrote: > Given current usage of this macro and the one below (here, in > V8ElementCustom.cpp and in generated bindings code), I'm pretty sure it's > pointless to call rethrowV8Exception() here now, since 'block' will re-throw the > caught exception on its own, and 'exceptionState' is a local that goes out of > scope when you return. > > So I reckon this could be > > if (UNLIKELY(block.HasCaught() || exceptionState.throwIfNeeded())) > return; > > instead. > > And if these macros were supposed to be useful in general code, I'd assume the > user would expect the resulting exception to be in 'exceptionState' only, and > not already have been rethrown via the V8 API. > > But I'm of course quite new to this code. :-) Done.
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (right): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:72: V8RethrowTryCatchScope rethrow(exceptionCatcher); Is the re-throwing here (despite also returning the exception in exceptionState) a temporary measure to be able to land "nested TryCatch" fixes in V8 without regressions? If so, I'm fine with that of course, but could you then add a FIXME comment here saying that we shouldn't need to re-throw here?
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (right): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:72: V8RethrowTryCatchScope rethrow(exceptionCatcher); On 2014/06/17 13:46:10, Jens Lindström wrote: > Is the re-throwing here (despite also returning the exception in exceptionState) > a temporary measure to be able to land "nested TryCatch" fixes in V8 without > regressions? > > If so, I'm fine with that of course, but could you then add a FIXME comment here > saying that we shouldn't need to re-throw here? I need ReThrow at L80. The current code doesn't care about ExceptionState there and I don't want to change the behavior.
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (right): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:72: V8RethrowTryCatchScope rethrow(exceptionCatcher); On 2014/06/17 13:53:13, yhirano wrote: > On 2014/06/17 13:46:10, Jens Lindström wrote: > > Is the re-throwing here (despite also returning the exception in > exceptionState) > > a temporary measure to be able to land "nested TryCatch" fixes in V8 without > > regressions? > > > > If so, I'm fine with that of course, but could you then add a FIXME comment > here > > saying that we shouldn't need to re-throw here? > I need ReThrow at L80. > The current code doesn't care about ExceptionState there and I don't want to > change the behavior. Oh, I had missed that the function returned at L80 too. But isn't the problem that it throws the exception the wrong way there? It ought to throw using exceptionState.throwTypeError(), no? If you do that, does it work without the re-throwing then? This is a bug today (without this patch or any V8 changes): if an exception is thrown there, the calling algorithm (NodeIterator or TreeWalker) will continue scanning for a suitable node, calling this function for each node until it runs out of nodes. And then the exception is still thrown in the end, so everything seems fine and no-one notices that the calling algorithm actually ignored the exception and continued its traversal.
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (right): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:72: V8RethrowTryCatchScope rethrow(exceptionCatcher); On 2014/06/17 14:07:20, Jens Lindström wrote: > On 2014/06/17 13:53:13, yhirano wrote: > > On 2014/06/17 13:46:10, Jens Lindström wrote: > > > Is the re-throwing here (despite also returning the exception in > > exceptionState) > > > a temporary measure to be able to land "nested TryCatch" fixes in V8 without > > > regressions? > > > > > > If so, I'm fine with that of course, but could you then add a FIXME comment > > here > > > saying that we shouldn't need to re-throw here? > > I need ReThrow at L80. > > The current code doesn't care about ExceptionState there and I don't want to > > change the behavior. > > Oh, I had missed that the function returned at L80 too. > > But isn't the problem that it throws the exception the wrong way there? It ought > to throw using exceptionState.throwTypeError(), no? If you do that, does it work > without the re-throwing then? > > This is a bug today (without this patch or any V8 changes): if an exception is > thrown there, the calling algorithm (NodeIterator or TreeWalker) will continue > scanning for a suitable node, calling this function for each node until it runs > out of nodes. And then the exception is still thrown in the end, so everything > seems fine and no-one notices that the calling algorithm actually ignored the > exception and continued its traversal. OK, I changed L79 at PS6. It changes error messages a bit. I can avoid the message change by using es.rethrowV8Exception(V8ThrowException::createTypeError()), but I'm not sure which is better.
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (right): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:72: V8RethrowTryCatchScope rethrow(exceptionCatcher); On 2014/06/18 07:17:52, yhirano wrote: > On 2014/06/17 14:07:20, Jens Lindström wrote: > > On 2014/06/17 13:53:13, yhirano wrote: > > > On 2014/06/17 13:46:10, Jens Lindström wrote: > > > > Is the re-throwing here (despite also returning the exception in > > > exceptionState) > > > > a temporary measure to be able to land "nested TryCatch" fixes in V8 > without > > > > regressions? > > > > > > > > If so, I'm fine with that of course, but could you then add a FIXME > comment > > > here > > > > saying that we shouldn't need to re-throw here? > > > I need ReThrow at L80. > > > The current code doesn't care about ExceptionState there and I don't want to > > > change the behavior. > > > > Oh, I had missed that the function returned at L80 too. > > > > But isn't the problem that it throws the exception the wrong way there? It > ought > > to throw using exceptionState.throwTypeError(), no? If you do that, does it > work > > without the re-throwing then? > > > > This is a bug today (without this patch or any V8 changes): if an exception is > > thrown there, the calling algorithm (NodeIterator or TreeWalker) will continue > > scanning for a suitable node, calling this function for each node until it > runs > > out of nodes. And then the exception is still thrown in the end, so everything > > seems fine and no-one notices that the calling algorithm actually ignored the > > exception and continued its traversal. > OK, I changed L79 at PS6. It changes error messages a bit. > I can avoid the message change by using > es.rethrowV8Exception(V8ThrowException::createTypeError()), but I'm not sure > which is better. The new error message (looking at the *-expected.txt change) seems more informative, so that looks like a good change to me. Did you test whether the V8RethrowTryCatchScope is still needed in this function with this change?
https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (right): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:72: V8RethrowTryCatchScope rethrow(exceptionCatcher); On 2014/06/18 07:26:06, Jens Lindström wrote: > On 2014/06/18 07:17:52, yhirano wrote: > > On 2014/06/17 14:07:20, Jens Lindström wrote: > > > On 2014/06/17 13:53:13, yhirano wrote: > > > > On 2014/06/17 13:46:10, Jens Lindström wrote: > > > > > Is the re-throwing here (despite also returning the exception in > > > > exceptionState) > > > > > a temporary measure to be able to land "nested TryCatch" fixes in V8 > > without > > > > > regressions? > > > > > > > > > > If so, I'm fine with that of course, but could you then add a FIXME > > comment > > > > here > > > > > saying that we shouldn't need to re-throw here? > > > > I need ReThrow at L80. > > > > The current code doesn't care about ExceptionState there and I don't want > to > > > > change the behavior. > > > > > > Oh, I had missed that the function returned at L80 too. > > > > > > But isn't the problem that it throws the exception the wrong way there? It > > ought > > > to throw using exceptionState.throwTypeError(), no? If you do that, does it > > work > > > without the re-throwing then? > > > > > > This is a bug today (without this patch or any V8 changes): if an exception > is > > > thrown there, the calling algorithm (NodeIterator or TreeWalker) will > continue > > > scanning for a suitable node, calling this function for each node until it > > runs > > > out of nodes. And then the exception is still thrown in the end, so > everything > > > seems fine and no-one notices that the calling algorithm actually ignored > the > > > exception and continued its traversal. > > OK, I changed L79 at PS6. It changes error messages a bit. > > I can avoid the message change by using > > es.rethrowV8Exception(V8ThrowException::createTypeError()), but I'm not sure > > which is better. > > The new error message (looking at the *-expected.txt change) seems more > informative, so that looks like a good change to me. > > Did you test whether the V8RethrowTryCatchScope is still needed in this function > with this change? Its not needed and I removed it at PS6.
LGTM (non-owner) haraken? https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... File Source/bindings/v8/V8NodeFilterCondition.cpp (right): https://codereview.chromium.org/313033002/diff/20001/Source/bindings/v8/V8Nod... Source/bindings/v8/V8NodeFilterCondition.cpp:72: V8RethrowTryCatchScope rethrow(exceptionCatcher); On 2014/06/18 07:31:36, yhirano wrote: > On 2014/06/18 07:26:06, Jens Lindström wrote: > > On 2014/06/18 07:17:52, yhirano wrote: > > > On 2014/06/17 14:07:20, Jens Lindström wrote: > > > > On 2014/06/17 13:53:13, yhirano wrote: > > > > > On 2014/06/17 13:46:10, Jens Lindström wrote: > > > > > > Is the re-throwing here (despite also returning the exception in > > > > > exceptionState) > > > > > > a temporary measure to be able to land "nested TryCatch" fixes in V8 > > > without > > > > > > regressions? > > > > > > > > > > > > If so, I'm fine with that of course, but could you then add a FIXME > > > comment > > > > > here > > > > > > saying that we shouldn't need to re-throw here? > > > > > I need ReThrow at L80. > > > > > The current code doesn't care about ExceptionState there and I don't > want > > to > > > > > change the behavior. > > > > > > > > Oh, I had missed that the function returned at L80 too. > > > > > > > > But isn't the problem that it throws the exception the wrong way there? It > > > ought > > > > to throw using exceptionState.throwTypeError(), no? If you do that, does > it > > > work > > > > without the re-throwing then? > > > > > > > > This is a bug today (without this patch or any V8 changes): if an > exception > > is > > > > thrown there, the calling algorithm (NodeIterator or TreeWalker) will > > continue > > > > scanning for a suitable node, calling this function for each node until it > > > runs > > > > out of nodes. And then the exception is still thrown in the end, so > > everything > > > > seems fine and no-one notices that the calling algorithm actually ignored > > the > > > > exception and continued its traversal. > > > OK, I changed L79 at PS6. It changes error messages a bit. > > > I can avoid the message change by using > > > es.rethrowV8Exception(V8ThrowException::createTypeError()), but I'm not sure > > > which is better. > > > > The new error message (looking at the *-expected.txt change) seems more > > informative, so that looks like a good change to me. > > > > Did you test whether the V8RethrowTryCatchScope is still needed in this > function > > with this change? > > Its not needed and I removed it at PS6. Cool! :-)
LGTM. jl@: Your review is very helpful :)
The CQ bit was checked by yhirano@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yhirano@chromium.org/313033002/120001
Message was sent while issue was closed.
Change committed as 176411 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
