Fix "report the exception" in Custom Elements V1

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

Index: third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
index 6c7b1e3de4e39b0513efb14349dddf12a81ff0b4..58c6c6c10c2943f9f8b90482441f83ea1b7e159d 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
@@ -189,6 +189,69 @@ HTMLElement* ScriptCustomElementDefinition::createElementSync(
     return toHTMLElement(element);
 }
 
+static AccessControlStatus accessControlStatusFromScriptOriginOptions(
+    const v8::ScriptOriginOptions& scriptOriginOptions)
+{
+    if (scriptOriginOptions.IsOpaque())
+        return OpaqueResource;
+    if (scriptOriginOptions.IsSharedCrossOrigin())
+        return SharableCrossOrigin;
+    return NotSharableCrossOrigin;

[haraken, 2016/08/18 09:03:02]

Can you create a helper function and share the code with V8Initializer?

+}
+
+static void reportException(ScriptState* scriptState,

[haraken, 2016/08/18 09:03:03]

Would you move this function to V8ThrowException?

The best solution would be to add a helper function to V8ThrowException and
share the code with V8Initializer.

[Yuki, 2016/08/18 09:34:26]

I opposed putting this function in V8ThrowException, and then kojii@ moved this
out of V8ThrowException.

This function is named "reportException", but actually this function *dispatch
an ErrorEvent*, and never throw an exception.

It seems our codebase is a little bit confused between dispatching an ErrorEvent
and throwing an Exception.  You can dispatch multiple ErrorEvent-s at a time
although you can only throw a single exception.

+    v8::Local<v8::Value> exception,
+    const String& eventMessage,
+    std::unique_ptr<SourceLocation> location,
+    const v8::ScriptOriginOptions& scriptOriginOptions)
+{
+    // Report an exception:
+    // https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception
+    ErrorEvent* event = ErrorEvent::create(eventMessage, std::move(location),
+        &scriptState->world());
+    V8ErrorHandler::storeExceptionOnErrorEventWrapper(scriptState, event,
+        exception, scriptState->context()->Global());
+    scriptState->getExecutionContext()->reportException(event,
+        accessControlStatusFromScriptOriginOptions(scriptOriginOptions));
+}
+
+static void reportException(ScriptState* scriptState,
+    v8::Local<v8::Value> exception,
+    const String& message,
+    v8::Local<v8::Function> function)
+{
+    reportException(scriptState, exception, message,
+        SourceLocation::fromFunction(function),
+        function->GetScriptOrigin().Options());
+}
+
+static void reportException(ScriptState* scriptState,
+    ExceptionState& exceptionState,
+    v8::Local<v8::Function> function)
+{
+    DCHECK(exceptionState.hadException());
+    v8::Local<v8::Value> exception = exceptionState.getException();
+    const String& message = exceptionState.message();
+    if (!message.isEmpty()) {

[haraken, 2016/08/18 09:03:02]

It looks nasty to have this branch.

Wouldn't it be problematic that some exceptions are already reported to
JavaScript but other exceptions are cleared before being reported to JavaScript?

[kojii, 2016/08/18 10:15:14]

There's no place to report, this is similar to unhandled exception. What's
different in custom element is, C++ can throw by checking the return value of
JS. When non-JS such as parser calls custom elements, JS returns fine, but C++
wants to throw because the result is invalid, there's no parent JS to throw to.

I wish to set SourceLocation and ScriptOrigin to such exceptions but didn't find
a way to do so.

+        // The exception was created in C++ but was not thrown in JavaScript.
+        // It does not have proper SourceLocation nor ScriptOrigin, so use the
+        // given information.
+        reportException(scriptState, exception, message, function);
+    } else {
+        // The exception was thrown in JavaScript, caught, and set by
+        // rethrowV8Exception(). It has SourceLocation and ScriptOrigin.
+        v8::Isolate* isolate = scriptState->isolate();
+        v8::Local<v8::Message> message = v8::Exception::CreateMessage(
+            isolate, exception);
+        reportException(scriptState, exception,
+            toCoreStringWithNullCheck(message->Get()),
+            SourceLocation::fromMessage(isolate, message,
+                scriptState->getExecutionContext()),
+            message->GetScriptOrigin().Options());
+    }
+    exceptionState.clearException();
+}
+
 HTMLElement* ScriptCustomElementDefinition::createElementSync(
     Document& document, const QualifiedName& tagName)
 {
@@ -202,16 +265,14 @@ HTMLElement* ScriptCustomElementDefinition::createElementSync(
         "CustomElement", constructor(), isolate);
     HTMLElement* element = createElementSync(document, tagName, exceptionState);
 
-    if (exceptionState.hadException() || !element) {
+    if (exceptionState.hadException()) {
+        DCHECK(!element);
         // 7. If this step throws an exception, then report the exception, ...
-        {
-            v8::TryCatch tryCatch(isolate);
-            tryCatch.SetVerbose(true);
-            exceptionState.throwIfNeeded();
-        }
-
+        reportException(m_scriptState.get(), exceptionState,
+            constructor().As<v8::Function>());

[haraken, 2016/08/18 09:03:02]

Why do you pass in constructor()? Here you're handling an exception thrown
during createElementSync.

[kojii, 2016/08/18 10:15:14]

This code path comes from the document parser, when it tries to create an
element. Custom element checks the result of the constructor and it's invalid,
so the best SourceLocation would be the constructor.

Document.createElement() doesn't come to this code path, it returns the
exception in ExceptionState back to the binding, which will throw to the caller.

         return CustomElement::createFailedElement(document, tagName);
     }
+    DCHECK(element);
     return element;
 }
 
@@ -238,12 +299,12 @@ bool ScriptCustomElementDefinition::runConstructor(Element* element)
     if (result != element) {
         const String& message = "custom element constructors must call super() first and must "
             "not return a different object";
-        std::unique_ptr<SourceLocation> location = SourceLocation::fromFunction(constructor().As<v8::Function>());
         v8::Local<v8::Value> exception =  V8ThrowException::createDOMException(
             m_scriptState->isolate(),
             InvalidStateError,
             message);
-        fireErrorEvent(m_scriptState.get(), message, exception, std::move(location));
+        reportException(m_scriptState.get(), exception, message,
+            constructor().As<v8::Function>());
         return false;
     }
 
@@ -268,14 +329,6 @@ Element* ScriptCustomElementDefinition::runConstructor()
     return V8Element::toImplWithTypeCheck(isolate, result);
 }
 
-void ScriptCustomElementDefinition::fireErrorEvent(ScriptState* scriptState, const String& message, v8::Local<v8::Value> exception, std::unique_ptr<SourceLocation> location)
-{
-    ErrorEvent* event = ErrorEvent::create(message, std::move(location), &scriptState->world());
-    V8ErrorHandler::storeExceptionOnErrorEventWrapper(scriptState, event, exception, scriptState->context()->Global());
-    ExecutionContext* executionContext = scriptState->getExecutionContext();
-    executionContext->reportException(event, NotSharableCrossOrigin);
-}
-
 v8::Local<v8::Object> ScriptCustomElementDefinition::constructor() const
 {
     DCHECK(!m_constructor.isEmpty());