From 5169d318dede44ae1e2dfde452e65bc470bf7eba Mon Sep 17 00:00:00 2001 From: Pritesh Nandgaonkar Date: Mon, 15 Apr 2019 05:41:28 -0700 Subject: [PATCH] Fix the callstack sent from error reporting runnable Summary: The crash logged from `ErrorReporterRunnable` wasn't detailed. For example look at the following video. Also for some reason the `FlipperConnection` interface doesn't have an argument for crash name. Thus I changed the js side of the plugin to accept `crash.name` to be undefined. {F155526537} Reviewed By: passy Differential Revision: D14852561 fbshipit-source-id: 6daf9847535b4508fa312b4f940b014911aae2e5 --- android/src/main/cpp/sonar.cpp | 20 ++++++++++---- .../android/FlipperConnectionImpl.java | 3 +++ .../flipper/core/ErrorReportingRunnable.java | 9 ++++++- .../flipper/core/FlipperConnection.java | 3 +++ .../testing/FlipperConnectionMock.java | 5 ++++ .../inspector/InspectorFlipperPluginTest.java | 5 +++- src/plugins/crash_reporter/index.js | 26 +++++++++++++------ 7 files changed, 56 insertions(+), 15 deletions(-) diff --git a/android/src/main/cpp/sonar.cpp b/android/src/main/cpp/sonar.cpp index ac9cefa97..ed5634a66 100644 --- a/android/src/main/cpp/sonar.cpp +++ b/android/src/main/cpp/sonar.cpp @@ -153,10 +153,13 @@ class JFlipperConnectionImpl : public jni::HybridClasssend(std::move(method), json ? folly::parseJson(json->toJsonString()) : folly::dynamic::object()); } + void reportErrorWithMetadata( + const std::string reason, + const std::string stackTrace) { + _connection->error(reason, stackTrace); + } + void reportError(jni::alias_ref throwable) { - _connection->error(throwable->toString(), throwable->getStackTrace()->toString()); + _connection->error( + throwable->toString(), throwable->getStackTrace()->toString()); } void receive(const std::string method, jni::alias_ref receiver) { diff --git a/android/src/main/java/com/facebook/flipper/android/FlipperConnectionImpl.java b/android/src/main/java/com/facebook/flipper/android/FlipperConnectionImpl.java index 340bc6d3a..28159133f 100644 --- a/android/src/main/java/com/facebook/flipper/android/FlipperConnectionImpl.java +++ b/android/src/main/java/com/facebook/flipper/android/FlipperConnectionImpl.java @@ -44,6 +44,9 @@ class FlipperConnectionImpl implements FlipperConnection { public native void sendArray(String method, FlipperArray params); + @Override + public native void reportErrorWithMetadata(String reason, String stackTrace); + @Override public native void reportError(Throwable throwable); diff --git a/android/src/main/java/com/facebook/flipper/core/ErrorReportingRunnable.java b/android/src/main/java/com/facebook/flipper/core/ErrorReportingRunnable.java index 77356ea4e..ae27576e1 100644 --- a/android/src/main/java/com/facebook/flipper/core/ErrorReportingRunnable.java +++ b/android/src/main/java/com/facebook/flipper/core/ErrorReportingRunnable.java @@ -7,6 +7,9 @@ */ package com.facebook.flipper.core; +import java.io.PrintWriter; +import java.io.StringWriter; + public abstract class ErrorReportingRunnable implements Runnable { private final FlipperConnection mConnection; @@ -21,7 +24,11 @@ public abstract class ErrorReportingRunnable implements Runnable { runOrThrow(); } catch (Exception e) { if (mConnection != null) { - mConnection.reportError(e); + StringWriter sw = new StringWriter(); + PrintWriter pw = new PrintWriter(sw); + e.printStackTrace(pw); + String sStackTrace = sw.toString(); // stack trace as a string + mConnection.reportErrorWithMetadata(e.toString(), sStackTrace); } } finally { doFinally(); diff --git a/android/src/main/java/com/facebook/flipper/core/FlipperConnection.java b/android/src/main/java/com/facebook/flipper/core/FlipperConnection.java index fa20d6962..942723f9b 100644 --- a/android/src/main/java/com/facebook/flipper/core/FlipperConnection.java +++ b/android/src/main/java/com/facebook/flipper/core/FlipperConnection.java @@ -26,6 +26,9 @@ public interface FlipperConnection { */ void send(String method, FlipperArray params); + /** Report client error with reason and stacktrace as an argument */ + void reportErrorWithMetadata(String reason, String stackTrace); + /** Report client error */ void reportError(Throwable throwable); diff --git a/android/src/main/java/com/facebook/flipper/testing/FlipperConnectionMock.java b/android/src/main/java/com/facebook/flipper/testing/FlipperConnectionMock.java index 1d64e42d1..6dc6dbf53 100644 --- a/android/src/main/java/com/facebook/flipper/testing/FlipperConnectionMock.java +++ b/android/src/main/java/com/facebook/flipper/testing/FlipperConnectionMock.java @@ -47,6 +47,11 @@ public class FlipperConnectionMock implements FlipperConnection { paramList.add(params); } + @Override + public void reportErrorWithMetadata(String reason, String stackTrace) { + errors.add(new Throwable(reason)); + } + @Override public void reportError(Throwable throwable) { errors.add(throwable); diff --git a/android/src/test/java/com/facebook/flipper/plugins/inspector/InspectorFlipperPluginTest.java b/android/src/test/java/com/facebook/flipper/plugins/inspector/InspectorFlipperPluginTest.java index c35c0be12..ca653ef81 100644 --- a/android/src/test/java/com/facebook/flipper/plugins/inspector/InspectorFlipperPluginTest.java +++ b/android/src/test/java/com/facebook/flipper/plugins/inspector/InspectorFlipperPluginTest.java @@ -318,8 +318,11 @@ public class InspectorFlipperPluginTest { new FlipperObject.Builder().put("ids", new FlipperArray.Builder().put("test")).build(), responder); + assertThat(connection.errors.size(), equalTo(1)); assertThat( - connection.errors, CoreMatchers.hasItem(hasThrowableWithMessage("Unexpected null value"))); + connection.errors, + CoreMatchers.hasItem( + hasThrowableWithMessage("java.lang.RuntimeException: Unexpected null value"))); } private class TestNode { diff --git a/src/plugins/crash_reporter/index.js b/src/plugins/crash_reporter/index.js index 75126f5fb..c45af99be 100644 --- a/src/plugins/crash_reporter/index.js +++ b/src/plugins/crash_reporter/index.js @@ -532,15 +532,25 @@ export default class CrashReporterPlugin extends FlipperDevicePlugin< static getActiveNotifications = ( persistedState: PersistedState, ): Array => { - return persistedState.crashes.map((crash: Crash) => { + const filteredCrashes = persistedState.crashes.filter(crash => { + let ignore = !crash.name && !crash.reason; + if (ignore) { + console.error('Ignored the notification for the crash', crash); + } + return ignore; + }); + return filteredCrashes.map((crash: Crash) => { const id = crash.notificationID; - const title = `CRASH: ${truncate(crash.name, 50)} Reason: ${truncate( - crash.reason, - 50, - )}`; - const callstack = CrashReporterPlugin.trimCallStackIfPossible( - crash.callstack, - ); + let name: string = crash.name || crash.reason; + let title: string = 'CRASH: ' + truncate(name, 50); + title = `${ + name == crash.reason + ? title + : title + 'Reason: ' + truncate(crash.reason, 50) + }`; + const callstack = crash.callstack + ? CrashReporterPlugin.trimCallStackIfPossible(crash.callstack) + : 'No callstack available'; const msg = `Callstack: ${truncate(callstack, 200)}`; return { id,