Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 1 month ago by aandrey Modified:
6 years, 1 month ago Reviewers:
vsevik, pfeldman, yurys CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, blink-reviews-bindings_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, arv+blink, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionShow correct location of unhandled promise rejection messages when DevTools closed.
BUG=427954
R=yurys, pfeldman
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185363
Patch Set 1 #
Total comments: 12
Patch Set 2 : addressed #
Total comments: 4
Patch Set 3 : addressed #Patch Set 4 : rebased #Patch Set 5 : rebased #
Total comments: 3
Created: 6 years, 1 month ago
Messages
Total messages: 38 (12 generated)
Depends on https://codereview.chromium.org/696703002/ patch to V8.
aandrey@chromium.org changed reviewers: + vsevik@chromium.org
ping
https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... File LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html (right): https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html:18: m0.catch(function() { { on the next line https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html:49: function runTest() Why do you do this manually and not reuse inspector-test.js? https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... File Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:291: v8::Handle<v8::Value> resourceName = message->GetScriptOrigin().ResourceName(); Looks like a lot of copy-paste here. Can we at least extract scriptResourceName(window, scriptOrigin)? https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:295: v8::Handle<v8::StackTrace> stackTrace = message->GetStackTrace(); extract callStack(stackTrace, isolate)? https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:301: scriptId = 0; extract scriptId(callStack, scriptOrigin)? https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:330: scriptId = message->GetScriptOrigin().ScriptID()->Value(); This is not correct way to use scriptId parameter on console message. I see however that we do the same for non-promise exceptions and that this parameter is ignored later in the code flow. Looking at this I've just realized we do not report stack traces for worker exceptions at all which is unfortunate. You could disregard this comment in context of this patch.
https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... File LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html (right): https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html:49: function runTest() On 2014/11/07 06:26:38, vsevik wrote: > Why do you do this manually and not reuse inspector-test.js? I believe the intent was to run this test without inspector being opened first. @aandrey: to achieve this you should move this test to LayoutTests/inspector-enabled/ all tests under that folder are expected to launch DevTools front-end manually using testRunner. https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... File Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:330: scriptId = message->GetScriptOrigin().ScriptID()->Value(); On 2014/11/07 06:26:38, vsevik wrote: > This is not correct way to use scriptId parameter on console message. > I see however that we do the same for non-promise exceptions and that this > parameter is ignored later in the code flow. > Looking at this I've just realized we do not report stack traces for worker > exceptions at all which is unfortunate. > You could disregard this comment in context of this patch. Can you file a bug on this?
> I believe the intent was to run this test without inspector being opened first. > > @aandrey: to achieve this you should move this test to > LayoutTests/inspector-enabled/ all tests under that folder are expected to > launch DevTools front-end manually using testRunner. You should also open the inspector then and call dumpConsoleMessages()
On 2014/11/07 12:49:27, yurys wrote: > https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... > File LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html > https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... > Source/bindings/core/v8/V8Initializer.cpp:330: scriptId = > message->GetScriptOrigin().ScriptID()->Value(); > On 2014/11/07 06:26:38, vsevik wrote: > > This is not correct way to use scriptId parameter on console message. > > I see however that we do the same for non-promise exceptions and that this > > parameter is ignored later in the code flow. > > Looking at this I've just realized we do not report stack traces for worker > > exceptions at all which is unfortunate. > > You could disregard this comment in context of this patch. > > Can you file a bug on this? https://code.google.com/p/chromium/issues/detail?id=431274
PTAL https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... File LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html (right): https://codereview.chromium.org/693183002/diff/1/LayoutTests/inspector/consol... LayoutTests/inspector/console/console-uncaught-promise-no-inspector.html:49: function runTest() On 2014/11/07 12:49:27, yurys wrote: > On 2014/11/07 06:26:38, vsevik wrote: > > Why do you do this manually and not reuse inspector-test.js? > > I believe the intent was to run this test without inspector being opened first. > > @aandrey: to achieve this you should move this test to > LayoutTests/inspector-enabled/ all tests under that folder are expected to > launch DevTools front-end manually using testRunner. Done. https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... File Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:291: v8::Handle<v8::Value> resourceName = message->GetScriptOrigin().ResourceName(); On 2014/11/07 06:26:38, vsevik wrote: > Looks like a lot of copy-paste here. > Can we at least extract scriptResourceName(window, scriptOrigin)? Done. https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:295: v8::Handle<v8::StackTrace> stackTrace = message->GetStackTrace(); On 2014/11/07 06:26:39, vsevik wrote: > extract callStack(stackTrace, isolate)? Done. https://codereview.chromium.org/693183002/diff/1/Source/bindings/core/v8/V8In... Source/bindings/core/v8/V8Initializer.cpp:301: scriptId = 0; On 2014/11/07 06:26:38, vsevik wrote: > extract scriptId(callStack, scriptOrigin)? Acknowledged.
lgtm https://codereview.chromium.org/693183002/diff/20001/LayoutTests/inspector-en... File LayoutTests/inspector-enabled/console/console-uncaught-promise-no-inspector.html (right): https://codereview.chromium.org/693183002/diff/20001/LayoutTests/inspector-en... LayoutTests/inspector-enabled/console/console-uncaught-promise-no-inspector.html:67: var test = function() function test() ? https://codereview.chromium.org/693183002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/693183002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8Initializer.cpp:329: lineNumber = static_cast<unsigned>(message->GetLineNumber()); can we change line/columnNumber type to int to avoid these static_casts?
https://codereview.chromium.org/693183002/diff/20001/LayoutTests/inspector-en... File LayoutTests/inspector-enabled/console/console-uncaught-promise-no-inspector.html (right): https://codereview.chromium.org/693183002/diff/20001/LayoutTests/inspector-en... LayoutTests/inspector-enabled/console/console-uncaught-promise-no-inspector.html:67: var test = function() On 2014/11/10 12:40:10, yurys wrote: > function test() ? Done. https://codereview.chromium.org/693183002/diff/20001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/693183002/diff/20001/Source/bindings/core/v8/... Source/bindings/core/v8/V8Initializer.cpp:329: lineNumber = static_cast<unsigned>(message->GetLineNumber()); On 2014/11/10 12:40:10, yurys wrote: > can we change line/columnNumber type to int to avoid these static_casts? Done.
The CQ bit was checked by aandrey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693183002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
The CQ bit was checked by aandrey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693183002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35607)
The CQ bit was checked by aandrey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693183002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35666)
The CQ bit was checked by aandrey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693183002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35765)
The CQ bit was checked by aandrey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693183002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
Depends on https://codereview.chromium.org/711353002/ patch to V8.
https://codereview.chromium.org/693183002/diff/80001/Source/bindings/core/v8/... File Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/693183002/diff/80001/Source/bindings/core/v8/... Source/bindings/core/v8/V8Initializer.cpp:304: promiseRejectMessageQueue().append(PromiseRejectMessage(ScriptValue(scriptState, promise), resourceName, scriptId, lineNumber, columnNumber, callStack)); Please see https://code.google.com/p/chromium/issues/detail?id=432533 for some thoughts on why we should pass exception object, not a promise object here.
https://codereview.chromium.org/693183002/diff/80001/LayoutTests/inspector-en... File LayoutTests/inspector-enabled/console/console-uncaught-promise-no-inspector.html (right): https://codereview.chromium.org/693183002/diff/80001/LayoutTests/inspector-en... LayoutTests/inspector-enabled/console/console-uncaught-promise-no-inspector.html:69: InspectorTest.expandConsoleMessages(onExpanded); They should be useful even before expanding. https://codereview.chromium.org/693183002/diff/80001/LayoutTests/inspector/co... File LayoutTests/inspector/console/console-uncaught-promise-in-worker-expected.txt (right): https://codereview.chromium.org/693183002/diff/80001/LayoutTests/inspector/co... LayoutTests/inspector/console/console-uncaught-promise-in-worker-expected.txt:1: CONSOLE ERROR: line 3: Unhandled promise rejection The similar message is useful in case of uncaught exception outside of promise. Can we make it better here?
Yes, we should certainly improve here on console messaging. Let's discuss and do this work separately. I'm going to land this patch once the V8 change is pulled to Blink.
> Let's discuss and do this work separately. Sure, np.
The CQ bit was checked by aandrey@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/693183002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 185363 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
