Fix "report the exception" in Custom Elements V1

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "bindings/core/v8/ScriptCustomElementDefinition.h"
 
 #include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8BindingMacros.h"
 #include "bindings/core/v8/V8CustomElementsRegistry.h"
@@ skipping 171 matching lines @@
 
     // 6.1.3. through 6.1.9.
     checkConstructorResult(element, document, tagName, exceptionState);
     if (exceptionState.hadException())
         return nullptr;
 
     DCHECK_EQ(element->getCustomElementState(), CustomElementState::Custom);
     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)
 {
     ScriptState::Scope scope(m_scriptState.get());
     v8::Isolate* isolate = m_scriptState->isolate();
 
     // When invoked from "create an element for a token":
     // https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token
 
     ExceptionState exceptionState(ExceptionState::ConstructionContext,
         "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;
 }
 
 // https://html.spec.whatwg.org/multipage/scripting.html#upgrades
 bool ScriptCustomElementDefinition::runConstructor(Element* element)
 {
     if (!m_scriptState->contextIsValid())
         return false;
     ScriptState::Scope scope(m_scriptState.get());
     v8::Isolate* isolate = m_scriptState->isolate();
 
     // Step 5 says to rethrow the exception; but there is no one to
     // catch it. The side effect is to report the error.
     v8::TryCatch tryCatch(isolate);
     tryCatch.SetVerbose(true);
 
     Element* result = runConstructor();
 
     // To report exception thrown from runConstructor()
     if (tryCatch.HasCaught())
         return false;
 
     // To report InvalidStateError Exception, when the constructor returns some different object
     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;
     }
 
     return true;
 }
 
 Element* ScriptCustomElementDefinition::runConstructor()
 {
     v8::Isolate* isolate = m_scriptState->isolate();
     DCHECK(ScriptState::current(isolate) == m_scriptState);
     ExecutionContext* executionContext = m_scriptState->getExecutionContext();
     v8::Local<v8::Value> result;
     if (!v8Call(V8ScriptRunner::callAsConstructor(
         isolate,
         constructor(),
         executionContext,
         0,
         nullptr),
         result)) {
         return nullptr;
     }
     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());
     return m_constructor.newLocal(m_scriptState->isolate());
 }
 
 v8::Local<v8::Object> ScriptCustomElementDefinition::prototype() const
 {
     DCHECK(!m_prototype.isEmpty());
     return m_prototype.newLocal(m_scriptState->isolate());
@@ skipping 83 matching lines @@
         v8String(isolate, name.localName()),
         v8StringOrNull(isolate, oldValue),
         v8StringOrNull(isolate, newValue),
         v8String(isolate, name.namespaceURI()),
     };
     runCallback(m_attributeChangedCallback.newLocal(isolate), element,
         argc, argv);
 }
 
 } // namespace blink