binding: Always throws an exception in the current realm.

third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp

Index: third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp b/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp
index 2a5afedeebc50eb0294721cc4537b687f68e0cea..df3d9227f6a254cae0ec05f96fdb6fd560dcb198 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp
@@ -33,7 +33,9 @@
 
 namespace blink {
 
-static void domExceptionStackGetter(v8::Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Value>& info)
+namespace {
+
+void domExceptionStackGetter(v8::Local<v8::Name> name, const v8::PropertyCallbackInfo<v8::Value>& info)
 {
     v8::Isolate* isolate = info.GetIsolate();
     v8::Local<v8::Value> value;
@@ -41,123 +43,75 @@ static void domExceptionStackGetter(v8::Local<v8::Name> name, const v8::Property
         v8SetReturnValue(info, value);
 }
 
-static void domExceptionStackSetter(v8::Local<v8::Name> name, v8::Local<v8::Value> value, const v8::PropertyCallbackInfo<void>& info)
+void domExceptionStackSetter(v8::Local<v8::Name> name, v8::Local<v8::Value> value, const v8::PropertyCallbackInfo<void>& info)
 {
     v8::Maybe<bool> unused = info.Data().As<v8::Object>()->Set(info.GetIsolate()->GetCurrentContext(), v8AtomicString(info.GetIsolate(), "stack"), value);
     ALLOW_UNUSED_LOCAL(unused);
 }
 
-v8::Local<v8::Value> V8ThrowException::createDOMException(v8::Isolate* isolate, int ec, const String& sanitizedMessage, const String& unsanitizedMessage, const v8::Local<v8::Object>& creationContext)
+} // namespace
+
+v8::Local<v8::Value> V8ThrowException::createDOMException(v8::Isolate* isolate, ExceptionCode exceptionCode, const String& sanitizedMessage, const String& unsanitizedMessage)

[haraken, 2016/08/09 07:53:50]

Just to confirm: Is it guaranteed that the current implementation calls
createDOMException with a correct current context?

[Yuki, 2016/08/10 03:11:09]

I suppose so.  There are three use cases of v8::Context::Enter in Blink.
1. InjectedScript: They intentionally run the code in the specific context, so
it must be okay to throw in that context.
2. V8WrapperInstantiationScope: It carefully handles the cross-origin case, and
rethrows if necessary.
3. ScriptState::Scope: It must be okay to create an exception in the current
realm while ScriptState::Scope() is created on the stack.

In general, exceptions can be thrown in the current realm by V8, so we must be
prepared for an exception created in the current realm.  e.g.
v8::Exception::{Range,Reference,Syntax,Type,}Error have always been created in
the current realm.

 {
-    if (ec <= 0 || isolate->IsExecutionTerminating())
-        return v8Undefined();
-
-    ASSERT(ec == SecurityError || unsanitizedMessage.isEmpty());
-
-    if (ec == V8GeneralError)
-        return V8ThrowException::createGeneralError(isolate, sanitizedMessage);
-    if (ec == V8TypeError)
-        return V8ThrowException::createTypeError(isolate, sanitizedMessage);
-    if (ec == V8RangeError)
-        return V8ThrowException::createRangeError(isolate, sanitizedMessage);
-    if (ec == V8SyntaxError)
-        return V8ThrowException::createSyntaxError(isolate, sanitizedMessage);
-    if (ec == V8ReferenceError)
-        return V8ThrowException::createReferenceError(isolate, sanitizedMessage);
-
-    v8::Local<v8::Object> sanitizedCreationContext = creationContext;
-
-    // FIXME: Is the current context always the right choice?
-    ScriptState* scriptState = ScriptState::from(creationContext->CreationContext());
-    Frame* frame = toFrameIfNotDetached(scriptState->context());
-    if (!frame || !BindingSecurity::shouldAllowAccessToFrame(isolate, currentDOMWindow(isolate), frame, DoNotReportSecurityError)) {

[haraken, 2016/08/09 07:53:50]

Do you know why we had this security check? This CL makes sense but I want to
make sure that we're not removing code that shouldn't be removed.

[Yuki, 2016/08/10 03:11:10]

The old implementation was creating a new exception in the given context
(|creationContext|) so we need to check that the current realm has the right
access to that context.

Now, we always create an exception in the current realm, there is no need to
check.

[haraken, 2016/08/10 05:00:33]

Makes sense.

-        scriptState = ScriptState::current(isolate);
-        sanitizedCreationContext = scriptState->context()->Global();
+    DCHECK_GT(exceptionCode, 0);
+    DCHECK(exceptionCode == SecurityError || unsanitizedMessage.isNull());
+
+    if (isolate->IsExecutionTerminating())
+        return v8::Local<v8::Value>();
+
+    switch (exceptionCode) {
+    case V8GeneralError:
+        return createGeneralError(isolate, sanitizedMessage);
+    case V8TypeError:
+        return createTypeError(isolate, sanitizedMessage);
+    case V8RangeError:
+        return createRangeError(isolate, sanitizedMessage);
+    case V8SyntaxError:
+        return createSyntaxError(isolate, sanitizedMessage);
+    case V8ReferenceError:
+        return createReferenceError(isolate, sanitizedMessage);
     }
 
     v8::TryCatch tryCatch(isolate);
 
-    DOMException* domException = DOMException::create(ec, sanitizedMessage, unsanitizedMessage);
-    v8::Local<v8::Value> exception = toV8(domException, sanitizedCreationContext, isolate);
-
+    DOMException* domException = DOMException::create(exceptionCode, sanitizedMessage, unsanitizedMessage);
+    v8::Local<v8::Object> exceptionObj = toV8(domException, isolate->GetCurrentContext()->Global(), isolate).As<v8::Object>();
+    // Attach an Error object to the DOMException. This is then lazily used to
+    // get the stack value.
+    v8::Local<v8::Value> error = v8::Exception::Error(v8String(isolate, domException->message()));
     if (tryCatch.HasCaught()) {

[haraken, 2016/08/09 07:53:50]

Just help me understand: Who can throw an exception?

[Yuki, 2016/08/10 03:11:10]

OOM (out of handles or stack) in V8?

Probably the original author wanted to make it sure that this function *creates*
an exception, does not *throw*.

I didn't realize this unlikeliness.  May I remove this TryCatch block?

[haraken, 2016/08/10 05:00:33]

Yeah, let's remove it.

[Yuki, 2016/08/10 05:45:11]

Done.

-        ASSERT(exception.IsEmpty());
         return tryCatch.Exception();
     }
-    ASSERT(!exception.IsEmpty());
-
-    // Attach an Error object to the DOMException. This is then lazily used to get the stack value.
-    v8::Local<v8::Value> error = v8::Exception::Error(v8String(isolate, domException->message()));
-    ASSERT(!error.IsEmpty());
-    v8::Local<v8::Object> exceptionObject = exception.As<v8::Object>();
-    exceptionObject->SetAccessor(isolate->GetCurrentContext(), v8AtomicString(isolate, "stack"), domExceptionStackGetter, domExceptionStackSetter, error).ToChecked();
+    // |exceptionObj| and |error| must not be empty here because of no
+    // exception.
+    if (!v8CallBoolean(exceptionObj->SetAccessor(isolate->GetCurrentContext(), v8AtomicString(isolate, "stack"), domExceptionStackGetter, domExceptionStackSetter, error))) {

[haraken, 2016/08/09 07:53:50]

Is it possible that the SetAccessor() call fails?

[Yuki, 2016/08/10 03:11:10]

Unlikely.

[haraken, 2016/08/10 05:00:33]

Then let's use ToChecked.

[Yuki, 2016/08/10 05:45:11]

Done.

+        CHECK(tryCatch.HasCaught());
+        return tryCatch.Exception();
+    }
 
     auto privateError = V8PrivateProperty::getDOMExceptionError(isolate);
-    privateError.set(scriptState->context(), exceptionObject, error);
+    privateError.set(isolate->GetCurrentContext(), exceptionObj, error);
 
-    return exception;
+    return exceptionObj;
 }
 
-v8::Local<v8::Value> V8ThrowException::createGeneralError(v8::Isolate* isolate, const String& message)
-{
-    return v8::Exception::Error(v8String(isolate, message.isNull() ? "Error" : message));
+#define DEFINE_CREATE_AND_THROW_ERROR_FUNC(blinkErrorType, v8ErrorType, defaultMessage) \
+v8::Local<v8::Value> V8ThrowException::create##blinkErrorType(v8::Isolate* isolate, const String& message) \
+{ \
+    return v8::Exception::v8ErrorType(v8String(isolate, message.isNull() ? defaultMessage : message)); \
+} \
+\
+void V8ThrowException::throw##blinkErrorType(v8::Isolate* isolate, const String& message) \
+{ \
+    throwException(isolate, create##blinkErrorType(isolate, message));    \
 }
 
-v8::Local<v8::Value> V8ThrowException::throwGeneralError(v8::Isolate* isolate, const String& message)
-{
-    v8::Local<v8::Value> exception = V8ThrowException::createGeneralError(isolate, message);
-    return V8ThrowException::throwException(exception, isolate);
-}
+DEFINE_CREATE_AND_THROW_ERROR_FUNC(GeneralError, Error, "Error")

[haraken, 2016/08/09 07:53:50]

Shall we rename GeneralError to Error in a follow-up CL? I guess GeneralError is
not a speced error.

[Yuki, 2016/08/10 03:11:10]

Yes, will do.

+DEFINE_CREATE_AND_THROW_ERROR_FUNC(RangeError, RangeError, "Range error")
+DEFINE_CREATE_AND_THROW_ERROR_FUNC(ReferenceError, ReferenceError, "Reference error")
+DEFINE_CREATE_AND_THROW_ERROR_FUNC(SyntaxError, SyntaxError, "Syntax error")
+DEFINE_CREATE_AND_THROW_ERROR_FUNC(TypeError, TypeError, "Type error")
 
-v8::Local<v8::Value> V8ThrowException::createTypeError(v8::Isolate* isolate, const String& message)
-{
-    return v8::Exception::TypeError(v8String(isolate, message.isNull() ? "Type error" : message));
-}
-
-v8::Local<v8::Value> V8ThrowException::throwTypeError(v8::Isolate* isolate, const String& message)
-{
-    v8::Local<v8::Value> exception = V8ThrowException::createTypeError(isolate, message);
-    return V8ThrowException::throwException(exception, isolate);
-}
-
-v8::Local<v8::Value> V8ThrowException::createRangeError(v8::Isolate* isolate, const String& message)
-{
-    return v8::Exception::RangeError(v8String(isolate, message.isNull() ? "Range error" : message));
-}
-
-v8::Local<v8::Value> V8ThrowException::throwRangeError(v8::Isolate* isolate, const String& message)
-{
-    v8::Local<v8::Value> exception = V8ThrowException::createRangeError(isolate, message);
-    return V8ThrowException::throwException(exception, isolate);
-}
-
-v8::Local<v8::Value> V8ThrowException::createSyntaxError(v8::Isolate* isolate, const String& message)
-{
-    return v8::Exception::SyntaxError(v8String(isolate, message.isNull() ? "Syntax error" : message));
-}
-
-v8::Local<v8::Value> V8ThrowException::throwSyntaxError(v8::Isolate* isolate, const String& message)
-{
-    v8::Local<v8::Value> exception = V8ThrowException::createSyntaxError(isolate, message);
-    return V8ThrowException::throwException(exception, isolate);
-}
-
-v8::Local<v8::Value> V8ThrowException::createReferenceError(v8::Isolate* isolate, const String& message)
-{
-    return v8::Exception::ReferenceError(v8String(isolate, message.isNull() ? "Reference error" : message));
-}
-
-v8::Local<v8::Value> V8ThrowException::throwReferenceError(v8::Isolate* isolate, const String& message)
-{
-    v8::Local<v8::Value> exception = V8ThrowException::createReferenceError(isolate, message);
-    return V8ThrowException::throwException(exception, isolate);
-}
-
-v8::Local<v8::Value> V8ThrowException::throwException(v8::Local<v8::Value> exception, v8::Isolate* isolate)
-{
-    if (!isolate->IsExecutionTerminating())
-        isolate->ThrowException(exception);
-    return v8::Undefined(isolate);
-}
+#undef DEFINE_CREATE_AND_THROW_ERROR_FUNC
 
 } // namespace blink