Fix "report the exception" in Custom Elements V1

third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.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/ErrorEventDispatcher.h"
+
+#include "bindings/core/v8/ScriptController.h"
+#include "bindings/core/v8/SourceLocation.h"
+#include "bindings/core/v8/V8DOMWrapper.h"
+#include "bindings/core/v8/V8ErrorHandler.h"
+#include "core/events/ErrorEvent.h"
+#include "platform/EventDispatchForbiddenScope.h"
+
+namespace blink {
+
+// static
+AccessControlStatus ErrorEventDispatcher::accessControlStatusFromScriptOriginOptions(

[haraken, 2016/08/19 01:51:08]

I'd inline this function into dispatchErrorEvent().

I think you introduced this function to share some code with V8Initializer, but
it isn't that helpful to share only code to extract AccessControlStatus.
Instead, we should try to share dispatchErrorEvent() with V8Initializer.

However, it will require a couple of work, so you don't need to do that in this
CL; i.e., let's just inline this function into dispatchErrorEvent() and revert
the change in V8Initializer. We can refactor V8Initializer so that it can use
dispatchErrorEvent() in a separate CL.

+    const v8::ScriptOriginOptions& scriptOriginOptions)
+{
+    if (scriptOriginOptions.IsOpaque())
+        return OpaqueResource;
+    if (scriptOriginOptions.IsSharedCrossOrigin())
+        return SharableCrossOrigin;
+    return NotSharableCrossOrigin;
+}
+
+// Custom elements validates return values from JS and may throw. When it does
+// so in non-JS context such as parser, the exceptions can't be thrown to V8.
+// dispatchErrorEvent() dispatches such errors and report to console.

[haraken, 2016/08/19 01:51:08]

I'd remove custom-event specific comments from this file.

+// static
+void ErrorEventDispatcher::dispatchErrorEvent(ScriptState* scriptState,
+    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 ErrorEventDispatcher::dispatchErrorEvent(ScriptState* scriptState,

[haraken, 2016/08/19 01:51:08]

Can we remove this helper function and inline the logic to
ScriptCustomElementDefinition? The logic to tweak SourceLocation is a specific
thing to ScriptCustomElementDefinition and thus encapsulated in
ScriptCustomElementDefinition.

+    v8::Local<v8::Value> exception,
+    const String& message,
+    v8::Local<v8::Function> function)
+{
+    // Since it's C++ that validates the return value, and the caller is not JS,
+    // use the SourceLocation of the function that returned the invalid value.
+    dispatchErrorEvent(scriptState, exception, message,
+        SourceLocation::fromFunction(function),
+        function->GetScriptOrigin().Options());
+}
+
+// static
+void ErrorEventDispatcher::dispatchErrorEvent(ScriptState* scriptState,

[haraken, 2016/08/19 01:51:08]

Ditto. I'd move this function to ScriptCustomElementDefinition.

+    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/19 01:51:08]

This check still looks very hacky. Is there any better way to detect if the
exception was thrown by V8 or DOM?

What happens if you just execute line 73 - 83 for an exception thrown by DOM?

[kojii, 2016/08/19 03:20:39]

It has an empty SourceLocation and ScritOrigin, which is not shareable, so it
will be sanitized before dispatching the event. All detail information is lost
except "script error".

[haraken, 2016/08/19 05:28:56]

Isn't it an expected behavior? i.e., it makes sense to me that an exception
thrown by DOM doesn't have any SourceLocation and ScriptOrigin.

If you unconditionally use line 73 - 83, will it crash somewhere?

[dominicc (has gone to gerrit), 2016/08/19 06:41:59]

Noooo, I think it is way better if we provide the source information if we have
it. Which would you rather--find an error that occurs... anywhere in your entire
program... or on a specific callstack?

+        // The exception was created in C++ after returned from JS.
+        // It does not have proper SourceLocation nor ScriptOrigin.
+        dispatchErrorEvent(scriptState, exception, message, function);
+    } else {
+        // The exception was thrown in JavaScript, caught, and set to
+        // ExceptionState::rethrowV8Exception(). It can then only be thrown
+        // to pending exceptions, that does not go to messageHandler.
+        v8::Isolate* isolate = scriptState->isolate();
+        v8::Local<v8::Message> message = v8::Exception::CreateMessage(
+            isolate, exception);
+        dispatchErrorEvent(scriptState, exception,
+            toCoreStringWithNullCheck(message->Get()),
+            SourceLocation::fromMessage(isolate, message,
+                scriptState->getExecutionContext()),
+            message->GetScriptOrigin().Options());
+    }
+    exceptionState.clearException();
+}
+
+} // namespace blink