Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2244203002:
Fix "report the exception" in Custom Elements V1 (Closed)
DescriptionFix "report the exception" in Custom Elements V1
There are 2 places the spec uses "report the exception"[1]:
1. The document parser invokes constructors synchronously.
2. Upgrade.
Currently Blink has 2 different code paths for each, and each has
different issues:
1. The synchronous constructor uses SetVerbose(true), which works for
thrown exceptions, but ExceptionState::throwIfNeeded() only puts
the exception into pending exceptions, which will be cleared when
TryCatch scope exits.
2. Upgrade fires error events and calls
ExecutionContext::reportException() to avoid it, but it always uses
NotSharableCrossOrigin. This works in run-layout-test where the
origin has m_unversalAccess, but not in, for instance, file URL.
This patch unifies the 2 code paths.
[1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception
BUG=632940
Committed: https://crrev.com/8b406045bb585b6a4bc7808d15df7070ba6e93ca
Cr-Commit-Position: refs/heads/master@{#413421}
Patch Set 1 #Patch Set 2 : Still WIP #Patch Set 3 : Still WIP #Patch Set 4 : Still WIP #Patch Set 5 : Cleanup and tests #
Total comments: 8
Patch Set 6 : yukishiino review #Patch Set 7 : Moved reportException to ScriptCustomElementDefinition.cpp #
Total comments: 2
Patch Set 8 : yukishiino review #
Total comments: 7
Patch Set 9 : Move to V8ErrorHandler along with messageHandlerInMainThread from V8Initializer #Patch Set 10 : Moved to a new class V8MessageHandler #Patch Set 11 : Renamed to ErrorEventDispatcher #
Total comments: 8
Patch Set 12 : dominicc review on tests at PS5 #Patch Set 13 : New approach to use TryCatch::ReThrow #
Total comments: 13
Patch Set 14 : More try-and-errors for new approach #Patch Set 15 : more try and errors #Patch Set 16 : Add 6 possible approaches #Patch Set 17 : Add approach 7 #Patch Set 18 : Took approach 7 and cleanup #
Total comments: 7
Patch Set 19 : haraken/yukishiino reviews #Messages
Total messages: 91 (40 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP: ce-error BUG=632940 ========== to ========== Fix "report the exception" in Custom Elements V1 There are 2 places the Custom Elements V1 spec uses "report the exception"[1]: 1. The document parser invokes constructors synchronously. 2. Upgrade. Currently Blink has 2 different code paths for each, and each has different issues: 1. The synchronous constructor uses SetVerbose(true), which works for thrown exceptions, but ExceptionState::throwIfNeeded() only puts the exception into pending exceptions, which will be cleared when TryCatch scope exits. 2. Upgrade fires error events and calls ExecutionContext::reportException() to avoid it, but it always uses NotSharableCrossOrigin. This works in run-layout-test where the origin has m_unversalAccess, but not in, for instance, file URL. This patch unifies the 2 code paths into a new method, V8ThrowException::reportException(). [1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception BUG=632940 ==========
kojii@chromium.org changed reviewers: + dominicc@chromium.org, yukishiino@chromium.org
@yukishiino, PTAL, it looks like you're the expert of V8ThrowException.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
I think ExceptionState should be a kind of stack-allocated scope like v8::TryCatch, and we shouldn't add reportException() into ExceptionState. See haraken@'s effort to remove ExceptionState::throwIfNeeded(). https://codereview.chromium.org/2061113002/ Once this gets done, you don't need to call throwIfNeeded(). The code will be like the following. ExceptionState exceptionState(...); if (!doSomething(exceptionState)) { return; // |exceptionState| throws } I'm also uneasy that V8ThrowException will take a ScriptState. V8ThrowException is designed to be a thin wrapper of v8::Exception, which doesn't take a v8::Context, so you shouldn't need any ScriptState there. Plus, ErrorEvent is not an Exception.
On 2016/08/18 at 05:09:34, yukishiino wrote: > I think ExceptionState should be a kind of stack-allocated scope like v8::TryCatch, and we shouldn't add reportException() into ExceptionState. > > See haraken@'s effort to remove ExceptionState::throwIfNeeded(). > https://codereview.chromium.org/2061113002/ > Once this gets done, you don't need to call throwIfNeeded(). The code will be like the following. > ExceptionState exceptionState(...); > if (!doSomething(exceptionState)) { > return; // |exceptionState| throws > } > > I'm also uneasy that V8ThrowException will take a ScriptState. V8ThrowException is designed to be a thin wrapper of v8::Exception, which doesn't take a v8::Context, so you shouldn't need any ScriptState there. > > Plus, ErrorEvent is not an Exception. Hmm...what do you think then to add getException() and getMessage() to ExceptionState, and do the reporting in ScriptCustomElementDefinition? In this case, we do not want to throw if hadException(). Just report it and discard.
kojii@chromium.org changed reviewers: + haraken@chromium.org
+haraken By looking at haraken's CL closer, my code is in trouble, because I need to report and discard the exception in ExceptionState. How about still adding ExceptionState::reportException(), which clears the current exception so that haraken's CL be happy?
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated the CL to use the strategy above (to add ExceptionState::reportException()). Also added "clearException" at the end of "reportException()" for haraken's CL to work well. If still adding ExceptionState::reportException() does not look good, adding ExceptionState::getException() and doing all the logic in ScriptCustomElementDefinition is fine with me, please let me know which looks better.
IIUC, what you'd like to do is to dispatch an ErrorEvent instead of throwing an exception when something error (including an exception) happens, right? ExecutionContext::reportException() is a kind of misnaming. It actually dispatches an ErrorEvent, so it should have been named reportErrorEvent(). Then, I think you want to reportErrorEvent() instead of to reportException(). Dispatching an ErrorEvent and throwing an Exception are totally different things (and not directly related). On 2016/08/18 05:33:29, kojii wrote: > How about still adding ExceptionState::reportException(), which clears the > current exception so that haraken's CL be happy? With the above renaming, ExceptionState::reportErrorEvent() sounds very strange. Why ExceptionState should be responsible to dispatch an ErrorEvent? I think ExceptionState is a wrong place to put reportErrorEvent() in. On 2016/08/18 05:30:15, kojii wrote: > Hmm...what do you think then to add getException() and getMessage() to > ExceptionState, and do the reporting in ScriptCustomElementDefinition? In this > case, we do not want to throw if hadException(). Just report it and discard. Adding getException() is fine. message() is already defined. You may need a minor refactoring on ExceptionState and it's fine. It's also okay to make a utility function that converts an exception into an ErrorEvent somewhere appropriate. My guts feeling is that we may want an ErrorEvent constructor that takes an Exception, but not sure. It can be an isolated(stand-alone) utility function.
On 2016/08/18 at 06:10:13, yukishiino wrote: > IIUC, what you'd like to do is to dispatch an ErrorEvent instead of throwing an exception when something error (including an exception) happens, right? Yes, the spec defines "report exception" and I'd like to implement this. https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception > Adding getException() is fine. message() is already defined. You may need a minor refactoring on ExceptionState and it's fine. > > It's also okay to make a utility function that converts an exception into an ErrorEvent somewhere appropriate. My guts feeling is that we may want an ErrorEvent constructor that takes an Exception, but not sure. It can be an isolated(stand-alone) utility function. Thank you for the advice, I'll add getException(). It does ErrorEvent and report to console, which is usually baked into unhandled exception handler, but when exceptions are put into ExceptionState, it cannot route to the unhandled exception handler. I'll move them to ScriptCustomElementDefinition for now, until we have a good place for it to live.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL. Added ExceptionState::getException() and moved reportException to ScriptCustomElementDefinition.
Description was changed from ========== Fix "report the exception" in Custom Elements V1 There are 2 places the Custom Elements V1 spec uses "report the exception"[1]: 1. The document parser invokes constructors synchronously. 2. Upgrade. Currently Blink has 2 different code paths for each, and each has different issues: 1. The synchronous constructor uses SetVerbose(true), which works for thrown exceptions, but ExceptionState::throwIfNeeded() only puts the exception into pending exceptions, which will be cleared when TryCatch scope exits. 2. Upgrade fires error events and calls ExecutionContext::reportException() to avoid it, but it always uses NotSharableCrossOrigin. This works in run-layout-test where the origin has m_unversalAccess, but not in, for instance, file URL. This patch unifies the 2 code paths into a new method, V8ThrowException::reportException(). [1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception BUG=632940 ========== to ========== Fix "report the exception" in Custom Elements V1 There are 2 places the Custom Elements V1 spec uses "report the exception"[1]: 1. The document parser invokes constructors synchronously. 2. Upgrade. Currently Blink has 2 different code paths for each, and each has different issues: 1. The synchronous constructor uses SetVerbose(true), which works for thrown exceptions, but ExceptionState::throwIfNeeded() only puts the exception into pending exceptions, which will be cleared when TryCatch scope exits. 2. Upgrade fires error events and calls ExecutionContext::reportException() to avoid it, but it always uses NotSharableCrossOrigin. This works in run-layout-test where the origin has m_unversalAccess, but not in, for instance, file URL. This patch unifies the 2 code paths. [1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception BUG=632940 ==========
LGTM. https://codereview.chromium.org/2244203002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (right): https://codereview.chromium.org/2244203002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:108: v8::Local<v8::Value> getException() { return m_exception.newLocal(m_isolate); } m_exception may be empty. m_exception.isEmpty() ? v8::Local<v8::Value>() : m_exception.newLocal(m_isolate) otherwise, CHECK(!m_exception.isEmpty()) or DCHECK at least.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for your review! https://codereview.chromium.org/2244203002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (right): https://codereview.chromium.org/2244203002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:108: v8::Local<v8::Value> getException() { return m_exception.newLocal(m_isolate); } On 2016/08/18 at 07:57:09, Yuki wrote: > m_exception may be empty. > m_exception.isEmpty() ? v8::Local<v8::Value>() : m_exception.newLocal(m_isolate) > > otherwise, CHECK(!m_exception.isEmpty()) or DCHECK at least. Done.
https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:199: return NotSharableCrossOrigin; Can you create a helper function and share the code with V8Initializer? https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:202: static void reportException(ScriptState* scriptState, 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. https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:235: if (!message.isEmpty()) { 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? https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:272: constructor().As<v8::Function>()); Why do you pass in constructor()? Here you're handling an exception thrown during createElementSync.
https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:202: static void reportException(ScriptState* scriptState, On 2016/08/18 09:03:03, haraken wrote: > > 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. 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:235: if (!message.isEmpty()) { On 2016/08/18 at 09:03:02, haraken wrote: > 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? 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. https://codereview.chromium.org/2244203002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:272: constructor().As<v8::Function>()); On 2016/08/18 at 09:03:02, haraken wrote: > Why do you pass in constructor()? Here you're handling an exception thrown during createElementSync. 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.
That said, I guess I should try harder to share the code to handle unhandled exceptions with V8Initializer. I'll think a bit more.
On 2016/08/18 10:54:17, kojii wrote: > That said, I guess I should try harder to share the code to handle unhandled > exceptions with V8Initializer. I'll think a bit more. Maybe we could add the helper functions to V8ErrorHandler.h.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/18 at 12:16:38, haraken wrote: > Maybe we could add the helper functions to V8ErrorHandler.h. Glad you commented what I thought ;) Moved to V8ErrorHandler, and since it does similar thing as messageHandlerInMainThread in V8Initializer, I moved the method too. WDYT? Is this bad?
I'm sorry for my late response (too late), but I'm afraid that V8ErrorHandler would be a wrong place... See V8ErrorHandler inherits from V8EventListener. V8ErrorHandler is designed to be an error event handler, not to be a namespace to hold static functions nor to be an event dispatcher. It's an event listener. IMHO, storeExceptionOnErrorEventWrapper() and loadExceptionFromErrorEventWrapper() are already misplaced in V8ErrorHandler. They should be placed somewhere else because they are not relevant to an event listener. I'm also uncomfortable with that you moved messageHandlerInMainThread() out of V8Initializer, but didn't move messageHandlerInWorker(). Not symmetric. Looks a bit ad-hoc. In general, I support this movement and thank a lot for this refactoring, but V8ErrorHandler wouldn't be the right place.
On 2016/08/18 14:55:38, Yuki wrote: > I'm sorry for my late response (too late), but I'm afraid that V8ErrorHandler > would be a wrong place... > > See V8ErrorHandler inherits from V8EventListener. V8ErrorHandler is designed to > be an error event handler, not to be a namespace to hold static functions nor to > be an event dispatcher. It's an event listener. > > IMHO, storeExceptionOnErrorEventWrapper() and > loadExceptionFromErrorEventWrapper() are already misplaced in V8ErrorHandler. > They should be placed somewhere else because they are not relevant to an event > listener. > > I'm also uncomfortable with that you moved messageHandlerInMainThread() out of > V8Initializer, but didn't move messageHandlerInWorker(). Not symmetric. Looks > a bit ad-hoc. > > In general, I support this movement and thank a lot for this refactoring, but > V8ErrorHandler wouldn't be the right place. I don't have any strong opinion on the place. V8ErrorHandle or V8Binding or a new file or whatever. My point is just that I want to share the code between custom elements and V8Initializer.
On 2016/08/18 14:55:38, Yuki wrote: > I'm sorry for my late response (too late), but I'm afraid that V8ErrorHandler > would be a wrong place... > > See V8ErrorHandler inherits from V8EventListener. V8ErrorHandler is designed to > be an error event handler, not to be a namespace to hold static functions nor to > be an event dispatcher. It's an event listener. > > IMHO, storeExceptionOnErrorEventWrapper() and > loadExceptionFromErrorEventWrapper() are already misplaced in V8ErrorHandler. > They should be placed somewhere else because they are not relevant to an event > listener. > > I'm also uncomfortable with that you moved messageHandlerInMainThread() out of > V8Initializer, but didn't move messageHandlerInWorker(). Not symmetric. Looks > a bit ad-hoc. Agreed. I'd prefer moving only error-event-related parts (more specifically, code that calls ErrorEvent::create + reportException) out to a helper function.
Ok...I tried a new static class, V8MessageHandler before I go to V8ErrorHandler. Let me try that.
On 2016/08/18 15:17:20, haraken wrote: > On 2016/08/18 14:55:38, Yuki wrote: > > I'm sorry for my late response (too late), but I'm afraid that V8ErrorHandler > > would be a wrong place... > > > > See V8ErrorHandler inherits from V8EventListener. V8ErrorHandler is designed > to > > be an error event handler, not to be a namespace to hold static functions nor > to > > be an event dispatcher. It's an event listener. > > > > IMHO, storeExceptionOnErrorEventWrapper() and > > loadExceptionFromErrorEventWrapper() are already misplaced in V8ErrorHandler. > > They should be placed somewhere else because they are not relevant to an event > > listener. > > > > I'm also uncomfortable with that you moved messageHandlerInMainThread() out of > > V8Initializer, but didn't move messageHandlerInWorker(). Not symmetric. > Looks > > a bit ad-hoc. > > Agreed. I'd prefer moving only error-event-related parts (more specifically, > code that calls ErrorEvent::create + reportException) out to a helper function. If it's only ErrorEvent::create + dispatchErrorEvent, I'm fine with V8Binding.h. Or, creating a new file would be a good option. ErrorEventDispatcher.h or something like that? Note that ErrorEvent is a DOM object and does not have a direct relationship with V8. Also note that it's an error reporter rather than an error handler, IMHO.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/18 at 15:25:11, yukishiino wrote: > On 2016/08/18 15:17:20, haraken wrote: > > > I'm also uncomfortable with that you moved messageHandlerInMainThread() out of > > > V8Initializer, but didn't move messageHandlerInWorker(). Not symmetric. > > Looks > > > a bit ad-hoc. > > > > Agreed. I'd prefer moving only error-event-related parts (more specifically, > > code that calls ErrorEvent::create + reportException) out to a helper function. It was reverted. > If it's only ErrorEvent::create + dispatchErrorEvent, I'm fine with V8Binding.h. > > Or, creating a new file would be a good option. ErrorEventDispatcher.h or something like that? > > Note that ErrorEvent is a DOM object and does not have a direct relationship with V8. Also note that it's an error reporter rather than an error handler, IMHO. Used "ErrorEventDispatcher", thank you for your suggestion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html (right): https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:27: <template id="common"> Is this worth it? It's only 4-5 lines of code to set up an onerror handler. But it is 6+ lines of code to specify this template (~4) and use it (~2). Maybe it is clearer to just write an onerror handler for each test? https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:32: errors.push(Array.prototype.slice.call(arguments)); Instead of pushing everything and digging out 4, which is a bit magic, could we write the argument list and store the argument we want? Can we assert anything useful about the other argument values? https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:64: assert_equals(error.name, rethrowErrorName); Can you check that the exact same object is rethrown? https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:85: <!-- ??? https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:102: assert_equals(error.name, 'TypeError'); Can we assert more precisely what these are? https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:109: test_with_window(w => { I think Google style says to always use the parens. Could we stick to that? https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:112: assert_type_error(w.errors[0][4]); I understand what these indices [0][4] are, but they look a bit magical, it would be easier to understand the test if we could avoid them.
https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp (right): https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:17: AccessControlStatus ErrorEventDispatcher::accessControlStatusFromScriptOriginOptions( 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. https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:29: // dispatchErrorEvent() dispatches such errors and report to console. I'd remove custom-event specific comments from this file. https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:48: void ErrorEventDispatcher::dispatchErrorEvent(ScriptState* scriptState, 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. https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:61: void ErrorEventDispatcher::dispatchErrorEvent(ScriptState* scriptState, Ditto. I'd move this function to ScriptCustomElementDefinition. https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:68: if (!message.isEmpty()) { 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?
https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp (right): https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:68: if (!message.isEmpty()) { On 2016/08/19 at 01:51:08, haraken wrote: > 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? 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".
https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html (right): https://codereview.chromium.org/2244203002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:64: assert_equals(error.name, rethrowErrorName); On 2016/08/18 at 23:16:45, dominicc wrote: > Can you check that the exact same object is rethrown? I could not find the spec defining it must be the exact same object. Can you? Comparing |name| was recommended to test rethrow at github PR[1] review from domenic. We might be rethrowing the exact same object, but I can't tell if it's a defined behavior or not... [1] https://github.com/w3c/web-platform-tests/pull/2940
https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp (right): https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:68: if (!message.isEmpty()) { On 2016/08/19 03:20:39, kojii wrote: > On 2016/08/19 at 01:51:08, haraken wrote: > > 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? > > 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". 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?
My $0.02 inline. https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp (right): https://codereview.chromium.org/2244203002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ErrorEventDispatcher.cpp:68: if (!message.isEmpty()) { On 2016/08/19 at 05:28:56, haraken wrote: > On 2016/08/19 03:20:39, kojii wrote: > > On 2016/08/19 at 01:51:08, haraken wrote: > > > 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? > > > > 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". > > 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. 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? > If you unconditionally use line 73 - 83, will it crash somewhere?
The test was updated according to dominicc review on PS5, great if you could have a look. For code, we had offline discussion, I'll try another approach in the next patch.
haraken@, yukishiino@, PS13 tried TryCatch::ReThrow(), it crashes at: [1:1:0819/162050:5802167048579:FATAL:ScriptWrappable.cpp(27)] Check failed: !wrapper.IsEmpty(). #0 0x7fb2d9dc6c2e base::debug::StackTrace::StackTrace() #1 0x7fb2d9de770b logging::LogMessage::~LogMessage() #2 0x7fb2d3ae4543 blink::ScriptWrappable::wrap() #3 0x7fb2d3aef79a blink::V8AbstractEventListener::handleEvent() #4 0x7fb2d3aef6d7 blink::V8AbstractEventListener::handleEvent() #5 0x7fb2d3f9efd4 blink::EventTarget::fireEventListeners() #6 0x7fb2d3f9e759 blink::EventTarget::fireEventListeners() #7 0x7fb2d3f92164 blink::EventDispatcher::dispatch() #8 0x7fb2d3f919bf blink::EventDispatcher::dispatchEvent() #9 0x7fb2d4448e96 blink::LocalDOMWindow::dispatchLoadEvent() #10 0x7fb2d4448ffd blink::LocalDOMWindow::documentWasClosed() #11 0x7fb2d3e870e4 blink::Document::implicitClose() #12 0x7fb2d4558d55 blink::FrameLoader::checkCompleted() #13 0x7fb2d4558c2a blink::FrameLoader::finishedParsing() #14 0x7fb2d3e92a85 blink::Document::finishedParsing() #15 0x7fb2d40d8a4f blink::HTMLDocumentParser::end() #16 0x7fb2d40d2c09 blink::HTMLDocumentParser::prepareToStopParsing() #17 0x7fb2d40d60df blink::HTMLDocumentParser::processTokenizedChunkFromBackgroundParser() #18 0x7fb2d40d3842 blink::HTMLDocumentParser::pumpPendingSpeculations() #19 0x7fb2d5d9032b blink::CancellableTaskFactory::CancellableTask::run() #20 0x7fb2d5da62a9 _ZN4base8internal7InvokerINS0_9BindStateIPFvSt10unique_ptrIN5blink13WebTaskRunner4TaskESt14default_deleteIS6_EEEJNS0_13PassedWrapperIS9_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #21 0x7fb2d9dc7766 base::debug::TaskAnnotator::RunTask() I'm going to debug it, but advises if any are appreciated.
https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:200: tryCatch.ReThrow(); This usage is simply wrong. https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:215: ExceptionState exceptionState(ExceptionState::ConstructionContext, Seems you can directly use v8::TryCatch here, and no need to use ExceptionState.
Sorry, this is my misunderstanding. Please ignore the following comments. On 2016/08/19 07:31:33, Yuki wrote: > https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp > (right): > > https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:200: > tryCatch.ReThrow(); > This usage is simply wrong. > > https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:215: > ExceptionState exceptionState(ExceptionState::ConstructionContext, > Seems you can directly use v8::TryCatch here, and no need to use ExceptionState.
lgtm Code looks good. Some points inline, mostly suggestions to make comments clearer. https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html (right): https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:8: The custom elements spec has 2 places where it [report the exception]: "it report" isn't right; it would need to be report*s*. But the spec text is "report the exception" so maybe rephrase it: ... places where it says to [report the exception]: BTW, you're referring to the HTML or DOM spec and not the custom elements spec? I'm not sure if "The custom elements spec" is accurate. 2 -> two? I think it looks better. "step 7" is fine because "step" kind of sets you up to expect the number. https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:10: 1. In the [create an element for a token], step 7. Omit 'the', or give the article something to name, eg In the [create an element for a token] algorithm, step 7. https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:13: set; i.e., the document parser. Might be more readable to avoid Latin, id est avoid "i.e.". https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:14: 2. In the [invoke custom element reactions], step 2.1. Same as above, the ... algorithm or simply drop "the" https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:16: Also it is likely that platform may run different code paths for: singular "platform" needs an article. The platform. Without an article it is confusing; maybe you mean different platforms may run different codepaths, but this is not a platform-specific issue. Why are you talking about the platform at all, here? That might be confused with OS X, Android, etc. HTML usually talks about the user agent, what about that? Maybe: There are different code paths when: 1. Author script throws an exception that is rethrown. 2. The user agent throws an exception. WDYT? https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:17: 1. When JavaScript throws and the platform rethrows. Again, grammar; "for when JavaScript throws" isn't right. Just put when: 1. JavaScript ... 2. The platform ... https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:33: errors.push({ Thank you. I know it is a bit verbose, but it is very readable and easy to understand. https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:38: error: error, Maybe omit the trailing comma? https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:46: function create_rethrow_error() { This is only used in one place; just inline it? https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:87: document.getElementById('constructor-throws').innerHTML; Indent two additional spaces. https://codereview.chromium.org/2244203002/diff/240001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/custom-elements/spec/report-the-exception.html:125: return []; // returning other objects than "this" is invalid. objects other than "this" I'm not 100% sure this is true in the synchronous parsing case or 'new' case, BTW; just for upgrade. But it is close enough.
PS14 tried: 1. v8call(object->Get()) but it doesn't help. 2. compileAndRunInternalScript() but still doesn't fire. I'll go home but will be back later today.
Added 6 approaches. * I still can't make 1-4 work. * 5 is workable, need to use a code in V8Initializer * 6 works good, but I'm afraid you don't like this approach...?
Patchset #16 (id:300001) has been deleted
Patchset #16 (id:320001) has been deleted
I found throwStackOverflowExceptionIfNeeded does similar thing as approach 6, so this might be ok, though throwStackOverflowExceptionIfNeeded does not set ScriptOrigin. So: * 6 does the same thing as throwStackOverflowExceptionIfNeeded * 7 modifies 6 to set ScriptOrigin of the constructor. Best if we could find a way to set ScriptOrigin to v8::Function, but I can't find it yet. It looks like the approach 7 is the best to me.
On 2016/08/20 14:46:26, kojii wrote: > I found throwStackOverflowExceptionIfNeeded does similar thing as approach 6, so > this might be ok, though throwStackOverflowExceptionIfNeeded does not set > ScriptOrigin. So: > * 6 does the same thing as throwStackOverflowExceptionIfNeeded > * 7 modifies 6 to set ScriptOrigin of the constructor. > > Best if we could find a way to set ScriptOrigin to v8::Function, but I can't > find it yet. > > It looks like the approach 7 is the best to me. The approach 6 and 7 look good to me overall. I still don't fully understand why you need to set a ScriptOrigin. Given that it's already guaranteed that you're throwing the exception to the same origin (because everything is happening inside the context you've entered), you don't need to set the ScriptOrigin, right? As far as I understand, that's why throwStackOverflowExceptionIfNeeded is not setting a ScriptOrigin.
On 2016/08/22 at 01:48:22, haraken wrote: > > The approach 6 and 7 look good to me overall. Great! I'll clean up approach 7. > I still don't fully understand why you need to set a ScriptOrigin. Given that it's already guaranteed that you're throwing the exception to the same origin (because everything is happening inside the context you've entered), you don't need to set the ScriptOrigin, right? As far as I understand, that's why throwStackOverflowExceptionIfNeeded is not setting a ScriptOrigin. With approach 6, the exception is thrown by a function created by Function::New, and this function doesn't belong to any Script. In this case, it's considered as null ScriptOrigin. In the code, it tests func->shared()->script()->IsScript(), which is false for Function::New, and returns an empty ScriptOrigin. https://cs.chromium.org/chromium/src/v8/src/api.cc?q=ScriptOrigin&sq=package:...
and FYI, this code checks the ScriptOrigin and sanitize as needed: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Execu...
On 2016/08/22 02:22:29, kojii wrote: > and FYI, this code checks the ScriptOrigin and sanitize as needed: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Execu... Just help me understand... (I think I'm misunderstanding something.) - What happens if the ScriptOrigin is null? Is the error message sanitized (and thus problematic)? Or is the error message not sanitized (and thus not problematic)? - I'm assuming that the error message is sanitized and thus problematic. If that's the case, why doesn't throwStackOverflowExceptionIfNeeded hit the same issue?
Description was changed from ========== Fix "report the exception" in Custom Elements V1 There are 2 places the Custom Elements V1 spec uses "report the exception"[1]: 1. The document parser invokes constructors synchronously. 2. Upgrade. Currently Blink has 2 different code paths for each, and each has different issues: 1. The synchronous constructor uses SetVerbose(true), which works for thrown exceptions, but ExceptionState::throwIfNeeded() only puts the exception into pending exceptions, which will be cleared when TryCatch scope exits. 2. Upgrade fires error events and calls ExecutionContext::reportException() to avoid it, but it always uses NotSharableCrossOrigin. This works in run-layout-test where the origin has m_unversalAccess, but not in, for instance, file URL. This patch unifies the 2 code paths. [1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception BUG=632940 ========== to ========== Fix "report the exception" in Custom Elements V1 There are 2 places the spec uses "report the exception"[1]: 1. The document parser invokes constructors synchronously. 2. Upgrade. Currently Blink has 2 different code paths for each, and each has different issues: 1. The synchronous constructor uses SetVerbose(true), which works for thrown exceptions, but ExceptionState::throwIfNeeded() only puts the exception into pending exceptions, which will be cleared when TryCatch scope exits. 2. Upgrade fires error events and calls ExecutionContext::reportException() to avoid it, but it always uses NotSharableCrossOrigin. This works in run-layout-test where the origin has m_unversalAccess, but not in, for instance, file URL. This patch unifies the 2 code paths. [1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception BUG=632940 ==========
On 2016/08/22 at 02:30:10, haraken wrote: > > - What happens if the ScriptOrigin is null? Is the error message sanitized (and thus problematic)? Or is the error message not sanitized (and thus not problematic)? The error will be sanitized only when CORS is not permitted, and thus problematic only in that case. The exception will then have NotSharableCrossOrigin. ExecutionContext::shouldSanitizeScriptError() calls SecurityOrigin::canRequestNoSuborigin() only when NotSharableCrossOrigin. When this function returns false, it will be sanitized. canRequestNoSuborigin() returns true for layout tests (m_universalAccess is true), or if isSameSchemeHostPort() is true. If the origin is file:, canRequstNoSuborigin() returns false. I didn't test but if the origin is HTTP, I suppose it follows CORS rules to determine whether to sanitize or not. Only in those cases it is problematic, and we would like to allow web developers to see where the error occurred for file: origin or CORS. > - I'm assuming that the error message is sanitized and thus problematic. If that's the case, why doesn't throwStackOverflowExceptionIfNeeded hit the same issue? I think it hits the same issue. If there were tests for it, we can try by running from file: URL without --run-layout-test option. Probably it just does not happen often, and thus is not worth the additional code to run for file: and CORS cases? Or, too scary to compile scripts when stack overflow is happening?
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/22 03:18:51, kojii wrote: > On 2016/08/22 at 02:30:10, haraken wrote: > > > > - What happens if the ScriptOrigin is null? Is the error message sanitized > (and thus problematic)? Or is the error message not sanitized (and thus not > problematic)? > > The error will be sanitized only when CORS is not permitted, and thus > problematic only in that case. > > The exception will then have NotSharableCrossOrigin. > ExecutionContext::shouldSanitizeScriptError() calls > SecurityOrigin::canRequestNoSuborigin() only when NotSharableCrossOrigin. When > this function returns false, it will be sanitized. canRequestNoSuborigin() > returns true for layout tests (m_universalAccess is true), or if > isSameSchemeHostPort() is true. > > If the origin is file:, canRequstNoSuborigin() returns false. I didn't test but > if the origin is HTTP, I suppose it follows CORS rules to determine whether to > sanitize or not. > > Only in those cases it is problematic, and we would like to allow web developers > to see where the error occurred for file: origin or CORS. Is this speced that way? Many things are already forbidden for file: origin and CORS. > > > - I'm assuming that the error message is sanitized and thus problematic. If > that's the case, why doesn't throwStackOverflowExceptionIfNeeded hit the same > issue? > > I think it hits the same issue. If there were tests for it, we can try by > running from file: URL without --run-layout-test option. > > Probably it just does not happen often, and thus is not worth the additional > code to run for file: and CORS cases? Or, too scary to compile scripts when > stack overflow is happening?
On 2016/08/22 at 04:48:17, haraken wrote: > > > > If the origin is file:, canRequstNoSuborigin() returns false. I didn't test but > > if the origin is HTTP, I suppose it follows CORS rules to determine whether to > > sanitize or not. > > > > Only in those cases it is problematic, and we would like to allow web developers > > to see where the error occurred for file: origin or CORS. > > Is this speced that way? Many things are already forbidden for file: origin and CORS. It is being speced in github issue[1][2]. The current plan has "optional script" to "report an error", and whether to give the constructor as the "optional script" for custom element case is not determined yet. We can follow the spec once it reached the consensus, but until then, I believe showing error location is quite important for web developer experience. [1] https://github.com/w3c/webcomponents/issues/547 [2] https://github.com/whatwg/html/issues/958#issuecomment-226309871
PTAL, if you're ok to go with what we believe is right, then follow the spec when it's defined differently.
LGTM https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:192: static void dispatchErrorEvent(v8::Isolate* isolate, origin => constructor ? https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:218: exceptionState.clearException(); Do you need to clear the exception here? https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:607: v8::Local<v8::Function> thrower = runCompiledInternalScript(isolate, script) It's silly that we have to compile the script every time. We should introduce a V8 API that enables us to set a ScriptOrigin on v8::Function. Would you file a bug and add a TODO? https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:609: callInternalFunction(thrower, thrower, 1, &exception, isolate); I'd prefer creating an array. v8::Local<v8::Value> args = { exception }; callInternalFunction(..., WTF_ARRAY_LENGTH(args), args, ...);
Thank you, will apply other comments, but one comment inline below: https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:218: exceptionState.clearException(); On 2016/08/22 at 06:36:47, haraken wrote: > Do you need to clear the exception here? Not today, but when we remove throwIfNeeded()[1], it will throw but will probably discarded (?). Since we processed it, clearing makes sense to me, but I'm ok to remove if this doesn't look good. [1] https://codereview.chromium.org/2061113002/
https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp (right): https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:218: exceptionState.clearException(); On 2016/08/22 06:36:47, haraken wrote: > > Do you need to clear the exception here? I think we'll need to clear the exception once haraken@'s CL to remove throwIfNeeded() gets landed. https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp (right): https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8ScriptRunner.cpp:593: // Use V8ThrowException instead of this function unless absolutely needed. Thanks for the caution, but it's better to put this caution in .h file.
On 2016/08/22 06:41:33, kojii wrote: > Thank you, will apply other comments, but one comment inline below: > > https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp > (right): > > https://codereview.chromium.org/2244203002/diff/380001/third_party/WebKit/Sou... > third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:218: > exceptionState.clearException(); > On 2016/08/22 at 06:36:47, haraken wrote: > > Do you need to clear the exception here? > > Not today, but when we remove throwIfNeeded()[1], it will throw but will > probably discarded (?). Since we processed it, clearing makes sense to me, but > I'm ok to remove if this doesn't look good. > > [1] https://codereview.chromium.org/2061113002/ Makes sense, thanks.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, dominicc@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2244203002/#ps400001 (title: "haraken/yukishiino reviews")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #19 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Fix "report the exception" in Custom Elements V1 There are 2 places the spec uses "report the exception"[1]: 1. The document parser invokes constructors synchronously. 2. Upgrade. Currently Blink has 2 different code paths for each, and each has different issues: 1. The synchronous constructor uses SetVerbose(true), which works for thrown exceptions, but ExceptionState::throwIfNeeded() only puts the exception into pending exceptions, which will be cleared when TryCatch scope exits. 2. Upgrade fires error events and calls ExecutionContext::reportException() to avoid it, but it always uses NotSharableCrossOrigin. This works in run-layout-test where the origin has m_unversalAccess, but not in, for instance, file URL. This patch unifies the 2 code paths. [1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception BUG=632940 ========== to ========== Fix "report the exception" in Custom Elements V1 There are 2 places the spec uses "report the exception"[1]: 1. The document parser invokes constructors synchronously. 2. Upgrade. Currently Blink has 2 different code paths for each, and each has different issues: 1. The synchronous constructor uses SetVerbose(true), which works for thrown exceptions, but ExceptionState::throwIfNeeded() only puts the exception into pending exceptions, which will be cleared when TryCatch scope exits. 2. Upgrade fires error events and calls ExecutionContext::reportException() to avoid it, but it always uses NotSharableCrossOrigin. This works in run-layout-test where the origin has m_unversalAccess, but not in, for instance, file URL. This patch unifies the 2 code paths. [1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception BUG=632940 Committed: https://crrev.com/8b406045bb585b6a4bc7808d15df7070ba6e93ca Cr-Commit-Position: refs/heads/master@{#413421} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/8b406045bb585b6a4bc7808d15df7070ba6e93ca Cr-Commit-Position: refs/heads/master@{#413421} |
