Rethrow exception when swallowing it is not intended.

Source/bindings/v8/V8NodeFilterCondition.cpp

Index: Source/bindings/v8/V8NodeFilterCondition.cpp
diff --git a/Source/bindings/v8/V8NodeFilterCondition.cpp b/Source/bindings/v8/V8NodeFilterCondition.cpp
index d8007c764733a6297614106a007f7e750e415a8b..5f9350096548a811ee4eabf30870c661940cf9dd 100644
--- a/Source/bindings/v8/V8NodeFilterCondition.cpp
+++ b/Source/bindings/v8/V8NodeFilterCondition.cpp
@@ -69,6 +69,7 @@ short V8NodeFilterCondition::acceptNode(Node* node, ExceptionState& exceptionSta
         return NodeFilter::FILTER_ACCEPT;
 
     v8::TryCatch exceptionCatcher;
+    V8RethrowTryCatchScope rethrow(exceptionCatcher);

[Jens Widell, 2014/06/17 13:46:10]

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?

[yhirano, 2014/06/17 13:53:13]

I need ReThrow at L80.
The current code doesn't care about ExceptionState there and I don't want to
change the behavior.

[Jens Widell, 2014/06/17 14:07:20]

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.

[yhirano, 2014/06/18 07:17:52]

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.

[Jens Widell, 2014/06/18 07:26:06]

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?

[yhirano, 2014/06/18 07:31:36]

Its not needed and I removed it at PS6.

[Jens Widell, 2014/06/18 07:36:30]

Cool! :-)

 
     v8::Handle<v8::Function> callback;
     if (filter->IsFunction())
@@ -89,7 +90,6 @@ short V8NodeFilterCondition::acceptNode(Node* node, ExceptionState& exceptionSta
     v8::Handle<v8::Value> result = ScriptController::callFunction(m_scriptState->executionContext(), callback, context, 1, info.get(), isolate);
 
     if (exceptionCatcher.HasCaught()) {
-        exceptionState.rethrowV8Exception(exceptionCatcher.Exception());

[Jens Widell, 2014/06/13 11:22:06]

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?

[Jens Widell, 2014/06/13 11:46:06]

See this TC: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3060

[yhirano, 2014/06/16 11:30:10]

Thanks, done.

[Jens Widell, 2014/06/16 11:56:39]

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.

         return NodeFilter::FILTER_REJECT;
     }