From 56f08fcf84ff7acc0da66b37d65f256f8837ca9d Mon Sep 17 00:00:00 2001
From: Moti Zilberman <moti@fb.com>
Date: Wed, 19 Jun 2019 09:09:57 -0700
Subject: [PATCH] Pop bridge frames from errors thrown by sync host function
 calls

Summary:
When a synchronous call from JS to native code throws an error, it doesn't include a useful stack trace from the native side. To improve error attribution, this diff pops the frames in `MessageQueue.js` and `NativeModules.js` from the stack traces of such errors. This uses the `error.framesToPop` convention understood by RN's global error handler.

For now we limit this to errors converted from C++ exceptions in host functions, since those are not likely to ever contain further JavaScript frames at the point where we catch them; if they did, it would violate our assumption that the top two frames of the stack are in the JS bridge code.

Reviewed By: cwdick

Differential Revision: D15805054

fbshipit-source-id: 8c1dd7c81b00b6a88e31473271889af1f88f7263
---
 Libraries/BatchedBridge/MessageQueue.js       | 14 ++++++++-
 .../__tests__/NativeModules-test.js           | 30 +++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/Libraries/BatchedBridge/MessageQueue.js b/Libraries/BatchedBridge/MessageQueue.js
index 57c9434f87f..7347df36121 100644
--- a/Libraries/BatchedBridge/MessageQueue.js
+++ b/Libraries/BatchedBridge/MessageQueue.js
@@ -182,7 +182,19 @@ class MessageQueue {
       );
     }
     this.processCallbacks(moduleID, methodID, params, onFail, onSucc);
-    return global.nativeCallSyncHook(moduleID, methodID, params);
+    try {
+      return global.nativeCallSyncHook(moduleID, methodID, params);
+    } catch (e) {
+      if (
+        typeof e === 'object' &&
+        e != null &&
+        typeof e.framesToPop === 'undefined' &&
+        /^Exception in HostFunction: /.test(e.message)
+      ) {
+        e.framesToPop = 2;
+      }
+      throw e;
+    }
   }
 
   processCallbacks(
diff --git a/Libraries/BatchedBridge/__tests__/NativeModules-test.js b/Libraries/BatchedBridge/__tests__/NativeModules-test.js
index a2fb9bb5d72..04909c8209b 100644
--- a/Libraries/BatchedBridge/__tests__/NativeModules-test.js
+++ b/Libraries/BatchedBridge/__tests__/NativeModules-test.js
@@ -211,6 +211,36 @@ describe('MessageQueue', function() {
       });
     });
 
+    it('throwing a "native" exception gets framesToPop = 2', function() {
+      global.nativeCallSyncHook = () => {
+        throw new Error('Exception in HostFunction: foo');
+      };
+      let error;
+      try {
+        NativeModules.RemoteModule1.syncMethod('paloAlto', 'menloPark');
+      } catch (e) {
+        error = e;
+      }
+      // We can't test this behaviour with `getLineFromFrame` because our mock
+      // function adds an extra frame, so check `framesToPop` directly instead.
+      expect(error.framesToPop).toBe(2);
+    });
+
+    it('throwing a "native" exception preserves framesToPop if set', function() {
+      global.nativeCallSyncHook = () => {
+        const e = new Error('Exception in HostFunction: foo');
+        e.framesToPop = 42;
+        throw e;
+      };
+      let error;
+      try {
+        NativeModules.RemoteModule1.syncMethod('paloAlto', 'menloPark');
+      } catch (e) {
+        error = e;
+      }
+      expect(error.framesToPop).toBe(42);
+    });
+
     it('returning a value', function() {
       global.nativeCallSyncHook = jest.fn(() => {
         return 'secondSucc';