createElement should not transmit exceptions but report them.

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 d782e5a2ab816bd76825718e8cf2c863e1816c2d..3807bd0158f146921f8c48ae9bf8a9c840f9b250 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
@@ -151,23 +151,46 @@ ScriptCustomElementDefinition::ScriptCustomElementDefinition(
       m_scriptState(scriptState),
       m_constructor(scriptState->isolate(), constructor) {}
 
-HTMLElement* ScriptCustomElementDefinition::createElementSync(
+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(ScriptState::current(m_scriptState->isolate()) == m_scriptState);
+  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);
+}
 
-  // Create an element
+HTMLElement* ScriptCustomElementDefinition::createElementSync(
+    Document& document,
+    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();
+
+  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);
@@ -175,51 +198,19 @@ HTMLElement* ScriptCustomElementDefinition::createElementSync(
     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;
-
-  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);
+    return handleCreateElementSyncException(document, tagName, isolate,
+                                            exceptionState);
   }
-  DCHECK(element);
-  return element;
+  DCHECK_EQ(element->getCustomElementState(), CustomElementState::Custom);
+  return toHTMLElement(element);
 }
 
 // https://html.spec.whatwg.org/multipage/scripting.html#upgrades