Rethrow exception when swallowing it is not intended.

Source/bindings/v8/V8NodeFilterCondition.cpp

 /*
  * Copyright (C) 2008, 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 51 matching lines @@
     v8::Isolate* isolate = m_scriptState->isolate();
     ASSERT(!m_scriptState->context().IsEmpty());
     v8::HandleScope handleScope(isolate);
     v8::Handle<v8::Value> filter = m_filter.newLocal(isolate);
 
     ASSERT(filter.IsEmpty() || filter->IsObject());
     if (filter.IsEmpty())
         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())
         callback = v8::Handle<v8::Function>::Cast(filter);
     else {
         v8::Local<v8::Value> value = filter->ToObject()->Get(v8AtomicString(isolate, "acceptNode"));
         if (value.IsEmpty() || !value->IsFunction()) {
             throwTypeError("NodeFilter object does not have an acceptNode function", isolate);
             return NodeFilter::FILTER_REJECT;
         }
         callback = v8::Handle<v8::Function>::Cast(value);
     }
 
     OwnPtr<v8::Handle<v8::Value>[]> info = adoptArrayPtr(new v8::Handle<v8::Value>[1]);
     v8::Handle<v8::Object> context = m_scriptState->context()->Global();
     info[0] = toV8(node, context, isolate);
 
     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;
     }
 
     ASSERT(!result.IsEmpty());
 
     return result->Int32Value();
 }
 
 void V8NodeFilterCondition::setWeakCallback(const v8::WeakCallbackData<v8::Value, V8NodeFilterCondition>& data)
 {
     data.GetParameter()->m_filter.clear();
 }
 
 } // namespace WebCore