createElement should not transmit exceptions but report them.

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/V8CustomElementRegistry.h"
@@ skipping 133 matching lines @@
     const v8::Local<v8::Object>& prototype,
     const v8::Local<v8::Function>& connectedCallback,
     const v8::Local<v8::Function>& disconnectedCallback,
     const v8::Local<v8::Function>& adoptedCallback,
     const v8::Local<v8::Function>& attributeChangedCallback,
     const HashSet<AtomicString>& observedAttributes)
     : CustomElementDefinition(descriptor, observedAttributes),
       m_scriptState(scriptState),
       m_constructor(scriptState->isolate(), constructor) {}
 
+static void dispatchErrorEvent(v8::Isolate* isolate,
+                               v8::Local<v8::Value> exception,
+                               v8::Local<v8::Object> constructor) {
+  v8::TryCatch tryCatch(isolate);
+  tryCatch.SetVerbose(true);
+  V8ScriptRunner::throwException(
+      isolate, exception, constructor.As<v8::Function>()->GetScriptOrigin());
+}
+
+HTMLElement* ScriptCustomElementDefinition::handleCreateElementSyncException(

[haraken, 2016/10/23 22:49:20]

Do you know why the spec requires to throw the exception synchronously? This
looks like a very special logic to the custom element. (See how hacky the
implementation of V8ScriptRunner::throwException is.)

[dominicc (has gone to gerrit), 2016/10/24 01:15:05]

What in particular is the problem?

Re: the spec--The spec changed and now requires us to report, not throw, the
exception. This method reports the exception with dispatchErrorEvent. rniwa
suggested this change so that synchronous element creation by the parser and
synchronous element creation by createElement reported exceptions in the same
way. I personally preferred to have createElement throw the exception, but not
strongly enough to get in the way of interoperable implementations. The
discussion is in the TPAC2016 Web Platform WG Monday minutes.

Re: V8ScriptRunner--It does look hacky. What does V8ScriptRunner::throwException
have to do with this?

+    Document& document,
+    const QualifiedName& tagName,
+    v8::Isolate* isolate,
+    ExceptionState& exceptionState) {
+  DCHECK(exceptionState.hadException());
+  // 6.1."If any of these subsubsteps threw an exception".1
+  // Report the exception.
+  dispatchErrorEvent(isolate, exceptionState.getException(), constructor());
+  exceptionState.clearException();
+  // ... .2 Return HTMLUnknownElement.
+  return CustomElement::createFailedElement(document, tagName);
+}
+
 HTMLElement* ScriptCustomElementDefinition::createElementSync(
     Document& document,
-    const QualifiedName& tagName,
-    ExceptionState& exceptionState) {
-  DCHECK(ScriptState::current(m_scriptState->isolate()) == m_scriptState);
+    const QualifiedName& tagName) {
+  ScriptState::Scope scope(m_scriptState.get());

[haraken, 2016/10/23 22:49:19]

Don't you need to check:

  if (!m_scriptState->contextIsValid())
    return;

?

[dominicc (has gone to gerrit), 2016/10/24 01:15:05]

Good catch, ty, fixed.

+  v8::Isolate* isolate = m_scriptState->isolate();
 
-  // Create an element
+  ExceptionState exceptionState(ExceptionState::ConstructionContext,
+                                "CustomElement", constructor(), isolate);
+
+  // Create an element with the synchronous custom elements flag set.
   // https://dom.spec.whatwg.org/#concept-create-element
-  // 6. If definition is non-null
-  // 6.1. If the synchronous custom elements flag is set:
-  // 6.1.2. Set result to Construct(C). Rethrow any exceptions.
 
   // Create an element and push to the construction stack.
   // V8HTMLElement::constructorCustom() can only refer to
   // window.document(), but it is different from the document here
-  // when it is an import document.  This is not exactly what the
-  // spec defines, but the public behavior matches to the spec.
+  // when it is an import document. This is not exactly what the
+  // spec defines, but the non-imports behavior matches to the spec.
   Element* element = createElementForConstructor(document);
   {
     ConstructionStackScope constructionStackScope(this, element);
     v8::TryCatch tryCatch(m_scriptState->isolate());
     element = runConstructor();
     if (tryCatch.HasCaught()) {
       exceptionState.rethrowV8Exception(tryCatch.Exception());
-      return nullptr;
+      return handleCreateElementSyncException(document, tagName, isolate,
+                                              exceptionState);
     }
   }
 
   // 6.1.3. through 6.1.9.
   checkConstructorResult(element, document, tagName, exceptionState);
-  if (exceptionState.hadException())
-    return nullptr;
-
+  if (exceptionState.hadException()) {
+    return handleCreateElementSyncException(document, tagName, isolate,
+                                            exceptionState);
+  }
   DCHECK_EQ(element->getCustomElementState(), CustomElementState::Custom);
   return toHTMLElement(element);
 }
 
-static void dispatchErrorEvent(v8::Isolate* isolate,
-                               v8::Local<v8::Value> exception,
-                               v8::Local<v8::Object> constructor) {
-  v8::TryCatch tryCatch(isolate);
-  tryCatch.SetVerbose(true);
-  V8ScriptRunner::throwException(
-      isolate, exception, constructor.As<v8::Function>()->GetScriptOrigin());
-}
-
-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()) {
-    DCHECK(!element);
-    // 7. If this step throws an exception, then report the exception, ...
-    dispatchErrorEvent(isolate, exceptionState.getException(), constructor());
-    exceptionState.clearException();
-    // and return HTMLUnknownElement.
-    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);
@@ skipping 123 matching lines @@
   v8::Local<v8::Value> argv[] = {
       v8String(isolate, name.localName()), v8StringOrNull(isolate, oldValue),
       v8StringOrNull(isolate, newValue),
       v8StringOrNull(isolate, name.namespaceURI()),
   };
   runCallback(m_attributeChangedCallback.newLocal(isolate), element,
               WTF_ARRAY_LENGTH(argv), argv);
 }
 
 }  // namespace blink