From b8458ef4e624c24962b4a2e16c749d0c165de10e Mon Sep 17 00:00:00 2001 From: Christian Petrov Date: Wed, 13 May 2020 15:43:54 +0200 Subject: [PATCH] Unify logging of rejection reasons Previously, rejections returned in event callbacks would break logging if their results didn't have a toString() function. Extract the formatting approach for rejections used in Promise to an util function and use it for event rejections as well. It can handle all kinds of reasons and ensures that stack traces will be printed when the rejection reason is not an error. Update the Promise rejection reason formatting to reuse code in util-stacktrace ensuring that the original stack trace will be printed in case the stack trace filter returns an empty stack trace (e.g. when the stack trace originates from Tabris.js). Change-Id: Ib856d5e2811138d6819a62898caeea8b736bc833 --- src/tabris/Events.js | 9 ++++-- src/tabris/Promise.js | 14 ++++----- src/tabris/util-stacktrace.js | 26 +++++++++++----- test/tabris/util-stacktrace.test.js | 48 ++++++++++++++++++++++++++++- 4 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/tabris/Events.js b/src/tabris/Events.js index 19fb54fb6..59e891991 100644 --- a/src/tabris/Events.js +++ b/src/tabris/Events.js @@ -3,6 +3,7 @@ import EventObject from './EventObject'; import {omit} from './util'; import {hint, toValueString} from './Console'; import {notify} from './symbols'; +import {formatPromiseRejectionReason} from './util-stacktrace'; export default { @@ -130,9 +131,11 @@ export default { for (const callback of this._callbacks[type]) { const value = callback.fn.call(callback.ctx || this, dispatchObject); if (value instanceof Promise) { - value.catch(ex => console.error( - `Listener for ${target.constructor.name} event "${type}" rejected:\n${ex.toString()}` - )); + value.catch(err => { + console.error( + `Listener for ${target.constructor.name} event "${type}" rejected: ${formatPromiseRejectionReason(err)}` + ); + }); } returnValues.push(value); } diff --git a/src/tabris/Promise.js b/src/tabris/Promise.js index ec0bb093a..e426c9621 100644 --- a/src/tabris/Promise.js +++ b/src/tabris/Promise.js @@ -23,7 +23,7 @@ * THE SOFTWARE. */ -import {formatStack} from './util-stacktrace'; +import {formatPromiseRejectionReason} from './util-stacktrace'; function asap(fn) { setTimeout(fn, 0); @@ -366,21 +366,19 @@ Promise._onHandle = (promise) => { delete rejections[promise[rejectionId]]; } }; -Promise._onReject = (promise, err) => { +Promise._onReject = (promise, error) => { if (promise._deferredState === 0) { // not yet handled promise[rejectionId] = lastRejectionId++; rejections[promise[rejectionId]] = { displayId: null, - error: err, + error, timeout: setTimeout(() => { const id = promise[rejectionId]; rejections[id].displayId = rejectionDisplayId++; rejections[id].logged = true; - let error = typeof err === 'undefined' ? '' : err; - if (!err || !err.stack) { - error += '\n' + formatStack(new Error().stack); - } - console.error(`Uncaught promise rejection (id: ${rejections[id].displayId}) ${error}`); + console.error( + `Uncaught promise rejection (id: ${rejections[id].displayId}) ${formatPromiseRejectionReason(error)}` + ); }, 0), logged: false }; diff --git a/src/tabris/util-stacktrace.js b/src/tabris/util-stacktrace.js index 21c96e8be..0e35f3cb9 100644 --- a/src/tabris/util-stacktrace.js +++ b/src/tabris/util-stacktrace.js @@ -7,9 +7,14 @@ const iosStackLineRegex = /^(.+)@(.*):([0-9]+):([0-9]+)/; const iosStackLineNoNameRegex = /(.*):([0-9]+):([0-9]+)/; const urlBaseRegEx = /^[a-z]+:\/\/[^/]+\//; -export function getStackTrace(error) { +/** + * @param {Error} error + * @param {boolean} errorStyleFormatting + * Prefix trace lines with "at", similarly to how logged errors are formatted in V8. + */ +export function getStackTrace(error, errorStyleFormatting = false) { try { - return getStackArray(error.stack).join('\n'); + return getStackArray(error.stack).map(line => errorStyleFormatting ? ' at ' + line : line).join('\n'); } catch (ex) { const minimalError = (ex && ex.constructor && ex.message) ? ex.constructor.name + ': ' + ex.message : ''; warn(`Could not process stack trace (${minimalError || ex}), printing original.`); @@ -40,12 +45,17 @@ export function formatError(error) { return error.constructor.name + ': ' + error.message + '\n' + stack; } -/** - * - * @param {string} stack - */ -export function formatStack(stack) { - return getStackArray(stack).map(line => ' at ' + line).join('\n'); +export function formatPromiseRejectionReason(reason) { + let result; + if (reason && typeof reason.toString === 'function') { + result = reason.toString(); + } else { + result = typeof reason === 'undefined' ? '' : reason + ''; + } + if (!reason || !reason.stack) { + result += '\n' + getStackTrace(new Error(), true); + } + return result; } export function getCurrentLine(error) { diff --git a/test/tabris/util-stacktrace.test.js b/test/tabris/util-stacktrace.test.js index 3683cf921..b44568a67 100644 --- a/test/tabris/util-stacktrace.test.js +++ b/test/tabris/util-stacktrace.test.js @@ -2,7 +2,13 @@ import {expect, stub, restore, mockTabris} from '../test'; import ClientMock from './ClientMock'; import {addWindowTimerMethods} from '../../src/tabris/WindowTimers'; import {create as createApp} from '../../src/tabris/App'; -import {getStackTrace, patchError, formatError, getCurrentLine} from '../../src/tabris/util-stacktrace'; +import { + getStackTrace, + patchError, + formatError, + getCurrentLine, + formatPromiseRejectionReason +} from '../../src/tabris/util-stacktrace'; import PromisePolyfill from '../../src/tabris/Promise'; describe('util-stacktrace', function() { @@ -224,6 +230,10 @@ showActionSheet (./dist/actionsheet.js:15:25)` expect(getStackTrace({stack: stacks[platform].production})).to.equal(stacks.expected.simplified); }); + it('prints stack trace with error-style formatting', function() { + expect(getStackTrace({stack: stacks[platform].production}, true)).to.equal(stacks.expected.simplifiedError); + }); + it('prints full stack trace in debug mode (non-minified)', function() { expect(getStackTrace({stack: stacks[platform].debug})).to.equal(stacks.expected.full); }); @@ -447,6 +457,42 @@ showActionSheet (./dist/actionsheet.js:15:25)` }); + describe('formatPromiseRejectionReason', function() { + + it('returns stack trace for undefined', function() { + const result = formatPromiseRejectionReason(); + + expect(result).to.match(/^\n {2}at .*util-stacktrace.js/); + }); + + it('returns stack trace and reason for null', function() { + const result = formatPromiseRejectionReason(null); + + expect(result).to.match(/^null\n {2}at .*util-stacktrace.js/); + }); + + it('returns stack trace and reason for string', function() { + const result = formatPromiseRejectionReason('foo'); + + expect(result).to.match(/^foo\n {2}at .*util-stacktrace.js/); + }); + + it('returns stack trace and reason for stringifiables', function() { + const stringifiable = {toString: () => 'foo'}; + const result = formatPromiseRejectionReason(stringifiable); + + expect(result).to.match(/^foo\n {2}at .*util-stacktrace.js/); + }); + + it('does not include stack if object has one', function() { + const stringifiable = {toString: () => 'foo', stack: 'bar'}; + const result = formatPromiseRejectionReason(stringifiable); + + expect(result).to.match(/^foo$/); + }); + + }); + }); });