+verwaest +jochen

Is it possible to add a V8 API to allow the embedder to set a ScriptOrigin on v8::Function::New?

it's not clear to me why you create a script to throw an exception instead of just throwing an exception via the API?

Context of Jochen's comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=721589#c12

My understanding is that this "hack" is here to allow the exception to be reported to embedder registered message handler:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp?type=cs&q=V8Initializer::MessageHandlerInMainThread&l=134

Using V8ThrowException::ThrowError would work if callstack includes Compile/Instantiate/Invoke (e.g. V8 func -> V8 binding -> Blink C++ -> V8ThrowException::ThrowError). We currently use V8ScriptRunner::ThrowException when we want to "report the error" from non-V8-eval stack.

why not invoke the message handler directly?

My knowledge on V8 is very limited, sorry if I'm not following correctly in advance.

> why not invoke the message handler directly?

We needed to throw a real JS exception, since it may be called from another JS that wants to catch. Want to go to message handler only if no try/catch in JS.

I found ThrowStackOverflowExceptionIfNeeded() does this by v8::Function::New():
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp?type=cs&q=throwStackOverflowExceptionIfNeeded&sq=package:chromium&l=120

so I tried the same technique, but then found that I can't set ScriptOrigin to the exception other than this method.

...does this answer to your question?

I think the right solution is to add a method to Isolate that throws an exception or calls the message handlers, and takes a script origin.

#11: Yeah, agree it'd be the ideal solution. Then both the custom element case and ThrowStackOverflowExceptionIfNeeded() can switch to it and avoid creating a function, do I understand correctly?

From following the link in  issue 721589 , it seems that this issue is causes an extremely easy-to-trigger problem with using the devtools together with modules.

I can reproduce some of this behavior at tip-of-tree: first, an HTML file, like:

```
<script type=module src="./module.js"></script>
```

And then the js file:

```
some syntax error
```

This results a SyntaxError that, when clicked on, shows me the "((e) = { throw e; })" source code. If indeed this is the cause, I think this is a P1, not a P3.

Having looked at this code a bit, I don't understand why the current code is set up as it is. kojii@ says in #10

"We needed to throw a real JS exception, since it may be called from another JS that wants to catch. Want to go to message handler only if no try/catch in JS."

Yet the way these exceptions work, I don't think they are catchable in JS, since they have a v8::TryCatch around them. The code for throwing errors in custom elements is:

```
static void DispatchErrorEvent(v8::Isolate* isolate,
                               v8::Local<v8::Value> exception,
                               v8::Local<v8::Object> constructor) {
  v8::TryCatch try_catch(isolate);
  try_catch.SetVerbose(true);
  V8ScriptRunner::ThrowException(
      isolate, exception, constructor.As<v8::Function>()->GetScriptOrigin());
}
```

I'm back to thinking that the best way to handle this would just be to have Blink call the message handler directly, rather than wire up a new V8 API, unless someone can show me an example where this codepath is used to actually throw an exception back to JS.

FWIW, on the custom elements side, https://chromium-review.googlesource.com/c/chromium/src/+/629299 passed the CQ dry run by calling into the message handler. I'm now waiting on results for the Module code path.

It seems like the remaining problem is that the inspector is not notified of exceptions reported in this way. I'm not yet sure whether it's worth round-tripping through the API for that purpose. 

We discuss this issue with Adam offline. And here is short summary from inspector side:
- inspector has two features about exceptions: we log them in console and we can pause execution on them and show current VM state,
- pause implementation requires existing of valid JavaScript state, we get all information from this state.
- so in case when exception is thrown from JavaScript code we have valid VM state and implement pause on exception internally inside V8, for exceptions without current JavaScript state (e.g. SyntaxError in <script>}</script>) we don't provide any kind of pause on exception. I think that import errors should not support pause on exception as well and http/tests/inspector/sources/inline-module-export-error.html is invalid test which can be removed.
- for exception logging, we log exception when V8Inspector::exceptionThrown is called, I believe that all exceptions from V8 first go to blink through V8Initializer::MessageHandlerInMainThread to check for window.onerror handlers and then exception is reported back by ThreadDebugger::exceptionThrown call so calling MessageHandlerInMainThread should be enough.
- I think that running fake script doesn't help inspector and can confuse our frontend when we receive two scriptParsed events for the same url with different content.

I remember one of reasons we did that. When js constructor ran successfully, but its return value was invalid, our C++ code throws an exception. Technically speaking, there's no JS that threw the exception, but it's the constructor who returned an invalid value.

So we wanted the exception to look like thrown by the constructor to authors. This isn't spec'ed since the location isn't well-defined, but we discussed on whatwg github and thought I'd the best for authors.

Does calling message handler directly still show the source correctly?

I'm sorry but I don't think I wore a test for this...

In manual testing the error reporting looks better. The old code generates the error, but also points to the beginning of the errant script (position 0, basically). The new code just points at the call to .define() (which is the code which triggered the upgrade).

See attached screenshots for the old and new behavior.

Deleted: old-exception.png 95.7 KB

	old-exception.png 95.7 KB View Download

Deleted: new-exception.png 96.3 KB

	new-exception.png 96.3 KB View Download

kojii and I looked at our tests (LayoutTests/custom-elements/spec) and we're pretty sure they're good and we don't want to break them.

new-exception.png looks wonderful.

I don't think we can just call the exception reporter directly there without messing up callstacks in the reported exception from document.createElement and document.createElementNS. Over Google's feedback against the proposal, a change was made to the custom elements spec so that createElement will not propagate exceptions from custom element constructors but instead report them. The reason for this change was that it made createElement consistent with parsing: all custom element constructor exceptions would be reported. Google's reasoning was that createElement can already throw exceptions (for invalid names) and throwing additional exceptions there would not be risky; and custom element constructor exceptions from 'new' must be caught and won't be reported (except through the usual toplevel unhandled exception reporter.) We lost that one.

IIRC the reason we did that TryCatch with verbose handling was, first, to call the exception reporter like everyone else; second, to glue the JavaScript callstacks together from the code above createElement (if any) and the point the system is generating the exception.

HTH, happy to answer questions. Custom element construction is unfortunately a bit intricate because of the many creation modes. It would be splendid if you could make that case in new-exception point to the constructor instead of the start of the script!

First, note that my proposed change doesn't affect any custom elements related tests (they all pass).

As for the "glue the JavaScript callstacks" bit, I believe that my call to v8::Exception::CreateMessage() is doing exactly that work, for the case when there's JS on the stack. See the implementation, which lives in v8::internal::Isolate:

https://cs.chromium.org/chromium/src/v8/src/isolate.cc?rcl=2021954c2b260e87989e9f543c198b51f49fb0e4&l=1684

In summary, as noted on https://chromium-review.googlesource.com/c/chromium/src/+/629299/6/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp#227, the only regression I see in testing the various custom element construction paths is in the case of an errant parser-created element: instead of pointing at the 0th source position in the script which defined that element, we just point at the HTML file where the error occurred. But as I said on the CL, I don't think this is a meaningful difference in practice.

The exception is visible to JavaScript in the dynamic import case - does this issue discuss a code path that is unrelated to dynamic import?

This is unrelated to dynamic import (since at the moment, dynamic import is unimplemented in Blink).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37078f2c1bf066c7e3e8a242d53d4ff67f106435

commit 37078f2c1bf066c7e3e8a242d53d4ff67f106435
Author: Adam Klein <adamk@chromium.org>
Date: Thu Aug 31 19:06:08 2017

Call MessageHandlerInMainThread directly to report exceptions from C++

V8ScriptRunner::ThrowException was used to report exceptions
that either occur with no script on the stack (for modules) or that
should not interrupt any script on the stack (for custom elements).
To carry out that duty, it created a new function from source and
passed it the exception to be thrown, and let that function throw
it.

This worked, but caused a variety of downstream problems. This patch
instead calls MessageHandlerInMainThread directly with the exception
to be reported (and renames the function to V8ScriptRunner::ThrowException).

Bug:  639739 , 755798
Change-Id: I3da012845737f817da796cf28a65e5b9741a2bf2
Reviewed-on: https://chromium-review.googlesource.com/629299
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498944}
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/LayoutTests/fast/dom/script-module-inline-error-gc-expected.txt
[delete] https://crrev.com/ad50e4512eeaee7ae1ac6d7390d3c9310f527600/third_party/WebKit/LayoutTests/http/tests/inspector/sources/inline-module-export-error-expected.txt
[delete] https://crrev.com/ad50e4512eeaee7ae1ac6d7390d3c9310f527600/third_party/WebKit/LayoutTests/http/tests/inspector/sources/inline-module-export-error.html
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/Source/bindings/core/v8/ScriptModule.cpp
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/Source/bindings/core/v8/ScriptModule.h
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.h
[modify] https://crrev.com/37078f2c1bf066c7e3e8a242d53d4ff67f106435/third_party/WebKit/Source/core/dom/ModulatorImpl.cpp

Issue 760823 remains open to further track the original intent of this feature.