Catch uncaught promise rejections from V8 and log to console.

Catch uncaught promise rejections from V8 and log to console.

Print error messages to DevTools console for rejected Promises that are unhandled by the platform end-of-turn.

BUG=393913
R=pfeldman, yurys

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183464

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183598

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183659

[aandrey, 2014-10-03 13:26:10]

Right now this just prints "Uncaught promise reject" to console w/o any useful
stuff, like error message, stack trace, error location. We need to modify v8 API
to provide that info.

[yurys, 2014-10-03 15:08:06]

We will need a test for this.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptValue.cpp (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptValue.cpp:44: v8::Local<v8::Value> value =
m_value->newLocal(isolate());
There should be a HandleScope.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptValue.h (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptValue.h:147:
ScriptValue(WTF::HashTableDeletedValueType) :
m_scriptState(WTF::HashTableDeletedValue) { }
If we init m_value with WTF::HashTableDeletedValue instead it would simplify
isEmptyValue in ScriptValueHashTraits below.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptValueTraits.h (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptValueTraits.h:21: static bool isEmptyValue(const
ScriptValue& value) { return value.isEmpty() &&
!value.isHashTableDeletedValue(); }
Why do we need !value.isHashTableDeletedValue(); ?

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:163: struct PromiseRejectMessage {
class?

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:185: static void
firePromiseRejectMessages()
Please wrap the new code into a class.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:190:
v8::Isolate::GetCurrent()->RemoveCallCompletedCallback(&firePromiseRejectMessages);
Why not add this callback once on the isolate initialization?

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:196:
RefPtrWillBeRawPtr<ScriptCallStack> callStack =
ScriptCallStack::create(callFrames);
= nullptr?

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:213: v8::Handle<v8::Value> data =
rejectValue.v8Value();
The whole idea we agreed on was to pass rejected value to DevTools console, we
should either convert it to string here or pass the value along with
corresponding console message.

Given that we don't support window.onerror reporting yet (waiting for spec) can
we create a ConsoleMessage here and pass rejected value as argument?

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:226: // Without having this ability,
it's hard to debug the private script because syntax errors
This code is shared between regular exception handler and promise rejections,
can you extract this into a method to avoid duplication?

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:261: PromiseRejectMessage* data = new
PromiseRejectMessage(promiseValue, ScriptValue(scriptState, value));
new should be wrapped with adoptPtr

[aandrey, 2014-10-06 10:52:57]

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptValue.cpp (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptValue.cpp:44: v8::Local<v8::Value> value =
m_value->newLocal(isolate());
On 2014/10/03 15:08:05, yurys wrote:
> There should be a HandleScope.

Done.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptValue.h (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptValue.h:147:
ScriptValue(WTF::HashTableDeletedValueType) :
m_scriptState(WTF::HashTableDeletedValue) { }
On 2014/10/03 15:08:05, yurys wrote:
> If we init m_value with WTF::HashTableDeletedValue instead it would simplify
> isEmptyValue in ScriptValueHashTraits below.

m_value->isEmpty(); will deref (-1) pointer

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptValueTraits.h (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptValueTraits.h:21: static bool isEmptyValue(const
ScriptValue& value) { return value.isEmpty() &&
!value.isHashTableDeletedValue(); }
On 2014/10/03 15:08:05, yurys wrote:
> Why do we need !value.isHashTableDeletedValue(); ?

Acknowledged.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:163: struct PromiseRejectMessage {
On 2014/10/03 15:08:06, yurys wrote:
> class?

Done.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:185: static void
firePromiseRejectMessages()
On 2014/10/03 15:08:06, yurys wrote:
> Please wrap the new code into a class.

I'd rather put this code together with the exception message hook, and see later
how to refactor both together.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:190:
v8::Isolate::GetCurrent()->RemoveCallCompletedCallback(&firePromiseRejectMessages);
On 2014/10/03 15:08:06, yurys wrote:
> Why not add this callback once on the isolate initialization?

So that we should not affect common use cases: we need this callback very
rarely.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:196:
RefPtrWillBeRawPtr<ScriptCallStack> callStack =
ScriptCallStack::create(callFrames);
On 2014/10/03 15:08:05, yurys wrote:
> = nullptr?

Done.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:213: v8::Handle<v8::Value> data =
rejectValue.v8Value();
On 2014/10/03 15:08:06, yurys wrote:
> The whole idea we agreed on was to pass rejected value to DevTools console, we
> should either convert it to string here or pass the value along with
> corresponding console message.
> 
> Given that we don't support window.onerror reporting yet (waiting for spec)
can
> we create a ConsoleMessage here and pass rejected value as argument?

Done. Passing ScriptArguments.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:226: // Without having this ability,
it's hard to debug the private script because syntax errors
On 2014/10/03 15:08:05, yurys wrote:
> This code is shared between regular exception handler and promise rejections,
> can you extract this into a method to avoid duplication?

Acknowledged.

https://codereview.chromium.org/625943002/diff/1/Source/bindings/core/v8/V8In...
Source/bindings/core/v8/V8Initializer.cpp:261: PromiseRejectMessage* data = new
PromiseRejectMessage(promiseValue, ScriptValue(scriptState, value));
On 2014/10/03 15:08:05, yurys wrote:
> new should be wrapped with adoptPtr

Done.

[pfeldman, 2014-10-06 13:11:59]

Capturing offline discussion comments here.

https://codereview.chromium.org/625943002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptValue.h (right):

https://codereview.chromium.org/625943002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptValue.h:147:
ScriptValue(WTF::HashTableDeletedValueType) :
m_scriptState(WTF::HashTableDeletedValue) { }
Lets store them in a vector instead.

https://codereview.chromium.org/625943002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/625943002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Initializer.cpp:185: static void
firePromiseRejectMessages()
Lets kick it from WebKit upon end of task instead.

https://codereview.chromium.org/625943002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Initializer.cpp:217:
scriptState->executionContext()->reportException(event.release(), scriptId,
callStack, arguments.release(), corsStatus, true);
Accessing frame console for main thread and capturing arguments there seems
simpler.

[aandrey, 2014-10-06 15:30:50]

Addressed. PTAL.

[pfeldman, 2014-10-06 19:40:13]

lgtm.

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:177: if
(scriptState->contextIsValid())
That's a really bad name. Lets swap the semantics in a separate patch.

[Jens Widell, 2014-10-07 05:22:23]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2014-10-07 05:22:23]

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:177: if
(scriptState->contextIsValid())
On 2014/10/06 19:40:13, pfeldman wrote:
> That's a really bad name. Lets swap the semantics in a separate patch.

https://codereview.chromium.org/622333002/

[yurys, 2014-10-07 05:32:44]

lgtm

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:195:
scriptState->executionContext()->addConsoleMessage(consoleMessage.release());
We should be careful not to call this on ScriptState wrapping debugger context
as the latter doesn't have ExecutionState and that would lead to a crash. Can
you check that promises created and reject during evaluation on debugger pause
are reported correctly and add a test for that case?

[aandrey, 2014-10-07 10:03:38]

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:177: if
(scriptState->contextIsValid())
On 2014/10/07 05:22:23, Jens Widell wrote:
> On 2014/10/06 19:40:13, pfeldman wrote:
> > That's a really bad name. Lets swap the semantics in a separate patch.
> 
> https://codereview.chromium.org/622333002/

Acknowledged.

https://codereview.chromium.org/625943002/diff/100001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Initializer.cpp:195:
scriptState->executionContext()->addConsoleMessage(consoleMessage.release());
On 2014/10/07 05:32:44, yurys (OOO) wrote:
> We should be careful not to call this on ScriptState wrapping debugger context
> as the latter doesn't have ExecutionState and that would lead to a crash. Can
> you check that promises created and reject during evaluation on debugger pause
> are reported correctly and add a test for that case?

Added a test, this case does not repro.
Seems like EndOfTaskRunner won't execute while we running debugger message loop.

Anyway, added the NULL check, since scriptState->executionContext()
theoretically can return 0.

[aandrey, 2014-10-07 10:03:58]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-10-07 10:04:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625943002/140001

[aandrey, 2014-10-07 10:33:47]

The CQ bit was unchecked by aandrey@chromium.org

[aandrey, 2014-10-07 10:34:44]

Waiting for V8 API changes: https://codereview.chromium.org/630373003/

[aandrey, 2014-10-07 12:55:40]

Ready to land after rolling V8 r24432.

[aandrey, 2014-10-08 20:11:08]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-10-08 20:12:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625943002/180001

[commit-bot: I haz the power, 2014-10-08 20:33:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-08 20:33:11]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[aandrey, 2014-10-09 13:11:38]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-10-09 13:12:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625943002/180001

[commit-bot: I haz the power, 2014-10-09 14:17:23]

Committed patchset #10 (id:180001) as 183464

[apavlov, 2014-10-09 15:10:25]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/648443002/ by apavlov@chromium.org.

The reason for reverting is: Broke
extensions_unittests.ModuleSystemTest.TestRequireAsyncFromContextWithNoModuleRegistry

See
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=....

[aandrey, 2014-10-13 07:21:34]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-10-13 07:22:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625943002/200001

[commit-bot: I haz the power, 2014-10-13 07:35:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 07:35:58]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/26813)

[aandrey, 2014-10-13 08:08:55]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-10-13 08:09:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625943002/380001

[commit-bot: I haz the power, 2014-10-13 09:10:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 09:10:12]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[aandrey, 2014-10-13 10:12:33]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-10-13 10:13:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625943002/520001

[commit-bot: I haz the power, 2014-10-13 11:15:15]

Committed patchset #13 (id:520001) as 183598

[cbiesinger, 2014-10-13 16:17:10]

A revert of this CL (patchset #13 id:520001) has been created in
https://codereview.chromium.org/650203003/ by cbiesinger@chromium.org.

The reason for reverting is: This is likely the cause of breakage for these
browser_tests:
ManageProfileUITest.Create_NameUpdateOnAvatarSelected
ManageProfileUITest.Create_NoNameUpdateOnAvatarSelectedAfterRevert
ManageProfileUITest.Create_NoNameUpdateOnAvatarSelectedAfterEdit


https://build.chromium.org/p/chromium.mac/builders/Mac10.6%20Tests%20%281%29/....

[aandrey, 2014-10-14 09:46:19]

Skipped the broken tests, see crbug.com/423267

[aandrey, 2014-10-14 09:46:25]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2014-10-14 09:46:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/625943002/520001

[commit-bot: I haz the power, 2014-10-14 09:47:57]

Committed patchset #13 (id:520001) as 183659