Skip to content

Commit

Permalink
Remove mapbuffer from early js error handling (facebook#43951)
Browse files Browse the repository at this point in the history 
Summary:
Pull Request resolved: facebook#43951

## Context
The **early js** error reporting pipeline catches javascript exceptions.

After errors are caught, the pipeline parses the exception, and puts the data into into a map buffer. 🤨

## Problems
We don't need to use a mapbuffer here: The structure of this exception data is known and never changes. (A map buffer is a type-unsafe bag of key/value pairs).

Instead, we could just use lower-level type-safe language primitives: regular C++ struct, and java class w/ fbjni.

## Changes
Migrate the **early js** error handling infra to C++ structs/fbjni.

## Impact
Now, there is no mapbuffer usage on iOS. We could re-introduce it when there is a need.

Changelog: [Internal]

Reviewed By: fkgozali

Differential Revision: D55265170

fbshipit-source-id: cda97633d4c6ccaad541e5d416067390fe6f61b2
  • Loading branch information
@RSNara @facebook-github-bot
RSNara authored and facebook-github-bot committed Apr 8, 2024
1 parent 5927165 commit b52c7aa
Show file tree
Hide file tree
Showing 14 changed files with 188 additions and 94 deletions.
14 changes: 14 additions & 0 deletions packages/react-native/ReactAndroid/api/ReactAndroid.api
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2830,6 +2830,20 @@ public abstract interface class com/facebook/react/interfaces/TaskInterface {
public abstract fun waitForCompletion (JLjava/util/concurrent/TimeUnit;)Z
}

public abstract interface class com/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedError {
public abstract fun getExceptionId ()I
public abstract fun getFrames ()Ljava/util/List;
public abstract fun getMessage ()Ljava/lang/String;
public abstract fun isFatal ()Z
}

public abstract interface class com/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedError$StackFrame {
public abstract fun getColumnNumber ()I
public abstract fun getFileName ()Ljava/lang/String;
public abstract fun getLineNumber ()I
public abstract fun getMethodName ()Ljava/lang/String;
}

public abstract interface class com/facebook/react/interfaces/fabric/ReactSurface {
public abstract fun clear ()V
public abstract fun detach ()V
Expand Down
36 changes: 34 additions & 2 deletions ...d/src/main/java/com/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,42 @@ package com.facebook.react.interfaces.exceptionmanager

import com.facebook.proguard.annotations.DoNotStripAny
import com.facebook.react.common.annotations.UnstableReactNativeAPI
import com.facebook.react.common.mapbuffer.ReadableMapBuffer
import java.util.ArrayList

@DoNotStripAny
@UnstableReactNativeAPI
public fun interface ReactJsExceptionHandler {
public fun reportJsException(errorMap: ReadableMapBuffer?)
@DoNotStripAny
public interface ParsedError {
@DoNotStripAny
public interface StackFrame {
public val fileName: String
public val methodName: String
public val lineNumber: Int
public val columnNumber: Int
}

public val frames: List<StackFrame>
public val message: String
public val exceptionId: Int
public val isFatal: Boolean
}

@DoNotStripAny
private data class ParsedStackFrameImpl(
override val fileName: String,
override val methodName: String,
override val lineNumber: Int,
override val columnNumber: Int,
) : ParsedError.StackFrame

@DoNotStripAny
private data class ParsedErrorImpl(
override val frames: ArrayList<ParsedStackFrameImpl>,
override val message: String,
override val exceptionId: Int,
override val isFatal: Boolean,
) : ParsedError

public fun reportJsException(errorMap: ParsedError)
}
48 changes: 44 additions & 4 deletions packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactExceptionManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,57 @@
#include <fbjni/fbjni.h>
#include <glog/logging.h>
#include <jni.h>
#include <iostream>

namespace facebook::react {

namespace {
class ParsedError : public facebook::jni::JavaClass<ParsedError> {
public:
static auto constexpr kJavaDescriptor =
"Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedError;";
};

class ParsedStackFrameImpl
: public facebook::jni::JavaClass<ParsedStackFrameImpl> {
public:
static auto constexpr kJavaDescriptor =
"Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedStackFrameImpl;";

static facebook::jni::local_ref<ParsedStackFrameImpl> create(
const JsErrorHandler::ParsedError::StackFrame& frame) {
return newInstance(
frame.fileName, frame.methodName, frame.lineNumber, frame.columnNumber);
}
};

class ParsedErrorImpl
: public facebook::jni::JavaClass<ParsedErrorImpl, ParsedError> {
public:
static auto constexpr kJavaDescriptor =
"Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler$ParsedErrorImpl;";

static facebook::jni::local_ref<ParsedErrorImpl> create(
const JsErrorHandler::ParsedError& error) {
auto stackFrames =
facebook::jni::JArrayList<ParsedStackFrameImpl>::create();
for (const auto& frame : error.frames) {
stackFrames->add(ParsedStackFrameImpl::create(frame));
}

return newInstance(
stackFrames, error.message, error.exceptionId, error.isFatal);
}
};

} // namespace

void JReactExceptionManager::reportJsException(
const JReadableMapBuffer::javaobject errorMapBuffer) {
const JsErrorHandler::ParsedError& error) {
static const auto method =
javaClassStatic()->getMethod<void(JReadableMapBuffer::javaobject)>(
javaClassStatic()->getMethod<void(jni::alias_ref<ParsedError>)>(
"reportJsException");
if (self() != nullptr) {
method(self(), errorMapBuffer);
method(self(), ParsedErrorImpl::create(error));
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactExceptionManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

#include <fbjni/fbjni.h>
#include <jni.h>
#include <react/common/mapbuffer/JReadableMapBuffer.h>
#include <jserrorhandler/JsErrorHandler.h>

namespace facebook::react {

Expand All @@ -19,7 +19,7 @@ class JReactExceptionManager
static auto constexpr kJavaDescriptor =
"Lcom/facebook/react/interfaces/exceptionmanager/ReactJsExceptionHandler;";

void reportJsException(const JReadableMapBuffer::javaobject errorMapBuffer);
void reportJsException(const JsErrorHandler::ParsedError& error);
};

} // namespace facebook::react
16 changes: 8 additions & 8 deletions packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactInstance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include <react/common/mapbuffer/JReadableMapBuffer.h>
#include <react/jni/JRuntimeExecutor.h>
#include <react/jni/JSLogging.h>
#include <react/renderer/mapbuffer/MapBuffer.h>
#include <react/runtime/BridgelessJSCallInvoker.h>
#include <react/runtime/BridgelessNativeMethodCallInvoker.h>
#include "JavaTimerRegistry.h"
Expand Down Expand Up @@ -52,13 +51,14 @@ JReactInstance::JReactInstance(
jsTimerExecutor->cthis()->setTimerManager(timerManager);

jReactExceptionManager_ = jni::make_global(jReactExceptionManager);
auto jsErrorHandlingFunc = [this](MapBuffer errorMap) noexcept {
if (jReactExceptionManager_ != nullptr) {
auto jErrorMap =
JReadableMapBuffer::createWithContents(std::move(errorMap));
jReactExceptionManager_->reportJsException(jErrorMap.get());
}
};
auto jsErrorHandlingFunc =
[weakJReactExceptionManager = jni::make_weak(jReactExceptionManager)](
const JsErrorHandler::ParsedError& error) mutable noexcept {
if (auto jReactExceptionManager =
weakJReactExceptionManager.lockLocal()) {
jReactExceptionManager->reportJsException(error);
}
};

jBindingsInstaller_ = jni::make_global(jBindingsInstaller);

Expand Down
65 changes: 33 additions & 32 deletions packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,14 @@
*/

#include "JsErrorHandler.h"
#include <react/renderer/mapbuffer/MapBufferBuilder.h>
#include <regex>
#include <sstream>
#include <string>
#include <vector>

namespace facebook::react {

using facebook::react::JSErrorHandlerKey;

static MapBuffer
static JsErrorHandler::ParsedError
parseErrorStack(const jsi::JSError& error, bool isFatal, bool isHermes) {
/**
* This parses the different stack traces and puts them into one format
Expand All @@ -43,63 +40,67 @@ parseErrorStack(const jsi::JSError& error, bool isFatal, bool isHermes) {
std::string line;
std::stringstream strStream(error.getStack());

auto errorObj = MapBufferBuilder();
std::vector<MapBuffer> frames;
std::vector<JsErrorHandler::ParsedError::StackFrame> frames;

while (std::getline(strStream, line, '\n')) {
auto frame = MapBufferBuilder();
auto searchResults = std::smatch{};

if (isHermes) {
if (std::regex_search(line, searchResults, REGEX_HERMES)) {
std::string str2 = std::string(searchResults[2]);
if (str2.compare("native")) {
frame.putString(kFrameFileName, std::string(searchResults[4]));
frame.putString(kFrameMethodName, std::string(searchResults[1]));
frame.putInt(kFrameLineNumber, std::stoi(searchResults[5]));
frame.putInt(kFrameColumnNumber, std::stoi(searchResults[6]));
frames.push_back(frame.build());
frames.push_back({
.fileName = std::string(searchResults[4]),
.methodName = std::string(searchResults[1]),
.lineNumber = std::stoi(searchResults[5]),
.columnNumber = std::stoi(searchResults[6]),
});
}
}
} else {
if (std::regex_search(line, searchResults, REGEX_GECKO)) {
frame.putString(kFrameFileName, std::string(searchResults[3]));
frame.putString(kFrameMethodName, std::string(searchResults[1]));
frame.putInt(kFrameLineNumber, std::stoi(searchResults[4]));
frame.putInt(kFrameColumnNumber, std::stoi(searchResults[5]));
frames.push_back({
.fileName = std::string(searchResults[3]),
.methodName = std::string(searchResults[1]),
.lineNumber = std::stoi(searchResults[4]),
.columnNumber = std::stoi(searchResults[5]),
});
} else if (
std::regex_search(line, searchResults, REGEX_CHROME) ||
std::regex_search(line, searchResults, REGEX_NODE)) {
frame.putString(kFrameFileName, std::string(searchResults[2]));
frame.putString(kFrameMethodName, std::string(searchResults[1]));
frame.putInt(kFrameLineNumber, std::stoi(searchResults[3]));
frame.putInt(kFrameColumnNumber, std::stoi(searchResults[4]));
frames.push_back({
.fileName = std::string(searchResults[2]),
.methodName = std::string(searchResults[1]),
.lineNumber = std::stoi(searchResults[3]),
.columnNumber = std::stoi(searchResults[4]),
});
} else {
continue;
}
frames.push_back(frame.build());
}
}
errorObj.putMapBufferList(kAllStackFrames, std::move(frames));
errorObj.putString(kErrorMessage, "EarlyJsError: " + error.getMessage());
// TODO: If needed, can increment exceptionId by 1 each time
errorObj.putInt(kExceptionId, 0);
errorObj.putBool(kIsFatal, isFatal);
return errorObj.build();

return {
.frames = std::move(frames),
.message = "EarlyJsError: " + error.getMessage(),
.exceptionId = 0,
.isFatal = isFatal,
};
}

JsErrorHandler::JsErrorHandler(
JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc) {
this->_jsErrorHandlingFunc = jsErrorHandlingFunc;
};
JsErrorHandler::JsErrorHandlingFunc jsErrorHandlingFunc)
: _jsErrorHandlingFunc(std::move(jsErrorHandlingFunc)){

};

JsErrorHandler::~JsErrorHandler() {}

void JsErrorHandler::handleJsError(const jsi::JSError& error, bool isFatal) {
// TODO: Current error parsing works and is stable. Can investigate using
// REGEX_HERMES to get additional Hermes data, though it requires JS setup.
MapBuffer errorMap = parseErrorStack(error, isFatal, false);
_jsErrorHandlingFunc(std::move(errorMap));
ParsedError parsedError = parseErrorStack(error, isFatal, false);
_jsErrorHandlingFunc(parsedError);
}

} // namespace facebook::react
32 changes: 17 additions & 15 deletions packages/react-native/ReactCommon/jserrorhandler/JsErrorHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,28 @@
#pragma once

#include <jsi/jsi.h>
#include <react/renderer/mapbuffer/MapBuffer.h>

namespace facebook::react {

enum JSErrorHandlerKey : uint16_t {
kFrameFileName = 0,
kFrameMethodName = 1,
kFrameLineNumber = 2,
kFrameColumnNumber = 3,
kAllStackFrames = 4,
kErrorMessage = 5,
kExceptionId = 6,
kIsFatal = 7
};

class JsErrorHandler {
public:
using JsErrorHandlingFunc = std::function<void(MapBuffer errorMap)>;

JsErrorHandler(JsErrorHandlingFunc jsErrorHandlingFunc);
struct ParsedError {
struct StackFrame {
std::string fileName;
std::string methodName;
int lineNumber;
int columnNumber;
};

std::vector<StackFrame> frames;
std::string message;
int exceptionId;
bool isFatal;
};

using JsErrorHandlingFunc = std::function<void(const ParsedError& error)>;

explicit JsErrorHandler(JsErrorHandlingFunc jsErrorHandlingFunc);
~JsErrorHandler();

void handleJsError(const jsi::JSError& error, bool isFatal);
Expand Down
1 change: 0 additions & 1 deletion packages/react-native/ReactCommon/jserrorhandler/React-jserrorhandler.podspec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,5 @@ Pod::Spec.new do |s|
s.dependency folly_dep_name, folly_version
s.dependency "React-jsi"
add_dependency(s, "React-debug")
add_dependency(s, "React-Mapbuffer")

end
22 changes: 16 additions & 6 deletions packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,22 @@ void ReactInstanceIntegrationTest::SetUp() {
auto timerManager =
std::make_shared<react::TimerManager>(std::move(mockRegistry));

auto jsErrorHandlingFunc = [](react::MapBuffer error) noexcept {
LOG(INFO) << "Error: \nFile: " << error.getString(react::kFrameFileName)
<< "\nLine: " << error.getInt(react::kFrameLineNumber)
<< "\nColumn: " << error.getInt(react::kFrameColumnNumber)
<< "\nMethod: " << error.getString(react::kFrameMethodName);
};
auto jsErrorHandlingFunc =
[](const JsErrorHandler::ParsedError& errorMap) noexcept {
LOG(INFO) << "[jsErrorHandlingFunc called]";
LOG(INFO) << "message: " << errorMap.message;
LOG(INFO) << "exceptionId: " << std::to_string(errorMap.exceptionId);
LOG(INFO) << "isFatal: "
<< std::to_string(static_cast<int>(errorMap.isFatal));
auto frames = errorMap.frames;
for (const auto& mapBuffer : frames) {
LOG(INFO) << "[Frame]" << std::endl
<< "\tfile: " << mapBuffer.fileName;
LOG(INFO) << "\tmethodName: " << mapBuffer.methodName;
LOG(INFO) << "\tlineNumber: " << std::to_string(mapBuffer.lineNumber);
LOG(INFO) << "\tcolumn: " << std::to_string(mapBuffer.columnNumber);
}
};

auto jsRuntimeFactory = std::make_unique<react::HermesInstance>();
std::unique_ptr<react::JSRuntime> runtime_ =
Expand Down
1 change: 1 addition & 0 deletions packages/react-native/ReactCommon/jsinspector-modern/tests/ReactNativeMocks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

#include "ReactNativeMocks.h"
#include <glog/logging.h>

namespace facebook::react::jsinspector_modern {

Expand Down
3 changes: 1 addition & 2 deletions packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include <react/utils/jsi-utils.h>
#include <iostream>
#include <memory>
#include <tuple>
#include <utility>

namespace facebook::react {
Expand All @@ -37,7 +36,7 @@ ReactInstance::ReactInstance(
: runtime_(std::move(runtime)),
jsMessageQueueThread_(jsMessageQueueThread),
timerManager_(std::move(timerManager)),
jsErrorHandler_(jsErrorHandlingFunc),
jsErrorHandler_(std::move(jsErrorHandlingFunc)),
hasFatalJsError_(std::make_shared<bool>(false)),
parentInspectorTarget_(parentInspectorTarget) {
RuntimeExecutor runtimeExecutor = [weakRuntime = std::weak_ptr(runtime_),
Expand Down
Loading

0 comments on commit b52c7aa

Please sign in to comment.