Fix error serialization issues (#637)

* Fix originalError serialization * Update coverage * Fix tests * Add tests * Simplify fix * Revert back to original error handling * Only include stack once
MetaMask · Jul 18, 2022 · 504f872 · 504f872
1 parent 2bca8b5
commit 504f872
Show file tree

Hide file tree

Showing 5 changed files with 199 additions and 23 deletions.
diff --git a/packages/controllers/src/services/node/NodeProcessExecutionService.test.ts b/packages/controllers/src/services/node/NodeProcessExecutionService.test.ts
@@ -198,8 +198,11 @@ describe('NodeProcessExecutionService', () => {
     // eslint-disable-next-line jest/prefer-strict-equal
     expect(await unhandledErrorPromise).toEqual({
       code: -32603,
-      data: { snapName: 'TestSnap' },
-      message: 'Unhandled promise rejection in snap.',
+      data: {
+        snapName: 'TestSnap',
+        stack: expect.any(String),
+      },
+      message: 'random error inside',
     });
 
     await service.terminateAllSnaps();

diff --git a/packages/controllers/src/services/node/NodeThreadExecutionService.test.ts b/packages/controllers/src/services/node/NodeThreadExecutionService.test.ts
@@ -198,8 +198,11 @@ describe('NodeThreadExecutionService', () => {
     // eslint-disable-next-line jest/prefer-strict-equal
     expect(await unhandledErrorPromise).toEqual({
       code: -32603,
-      data: { snapName: 'TestSnap' },
-      message: 'Unhandled promise rejection in snap.',
+      data: {
+        snapName: 'TestSnap',
+        stack: expect.any(String),
+      },
+      message: 'random error inside',
     });
 
     await service.terminateAllSnaps();

diff --git a/packages/execution-environments/jest.config.js b/packages/execution-environments/jest.config.js
@@ -6,10 +6,10 @@ module.exports = {
   coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'],
   coverageThreshold: {
     global: {
-      branches: 84.67,
-      functions: 89.52,
-      lines: 84.91,
-      statements: 85.14,
+      branches: 84.62,
+      functions: 92.38,
+      lines: 86.18,
+      statements: 86.38,
     },
   },
   moduleFileExtensions: ['js', 'json', 'jsx', 'ts', 'tsx', 'node'],

diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.test.ts b/packages/execution-environments/src/common/BaseSnapExecutor.test.ts
@@ -1,6 +1,6 @@
 // eslint-disable-next-line import/no-unassigned-import
 import 'ses';
-import { Duplex, DuplexOptions, Readable } from 'stream';
+import { Duplex, DuplexOptions, EventEmitter, Readable } from 'stream';
 import { JsonRpcResponse } from '@metamask/utils';
 import { JsonRpcRequest } from '../__GENERATED__/openrpc';
 import { BaseSnapExecutor } from './BaseSnapExecutor';
@@ -522,6 +522,183 @@ describe('BaseSnapExecutor', () => {
     });
   });
 
+  it('notifies execution service of out of band errors via unhandledrejection', async () => {
+    const CODE = `
+    module.exports.onRpcRequest = async () => 'foo';
+    `;
+    const executor = new TestSnapExecutor();
+    const emitter = new EventEmitter();
+
+    jest
+      .spyOn(window, 'addEventListener')
+      .mockImplementation((type, listener) =>
+        emitter.on(type, listener as any),
+      );
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      id: 1,
+      method: 'executeSnap',
+      params: [FAKE_SNAP_NAME, CODE, []],
+    });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      jsonrpc: '2.0',
+      id: 1,
+      result: 'OK',
+    });
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      id: 2,
+      method: 'snapRpc',
+      params: [
+        FAKE_SNAP_NAME,
+        FAKE_ORIGIN,
+        { jsonrpc: '2.0', method: '', params: [] },
+      ],
+    });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      id: 2,
+      jsonrpc: '2.0',
+      result: 'foo',
+    });
+
+    const testError = new Error('test error');
+    emitter.emit('unhandledrejection', { reason: testError });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      jsonrpc: '2.0',
+      error: {
+        code: -32603,
+        data: {
+          stack: testError.stack,
+          snapName: 'local:foo',
+        },
+        message: testError.message,
+      },
+    });
+  });
+
+  it('notifies execution service of out of band errors via unhandledrejection when event is error', async () => {
+    const CODE = `
+    module.exports.onRpcRequest = async () => 'foo';
+    `;
+    const executor = new TestSnapExecutor();
+    const emitter = new EventEmitter();
+
+    jest
+      .spyOn(window, 'addEventListener')
+      .mockImplementation((type, listener) =>
+        emitter.on(type, listener as any),
+      );
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      id: 1,
+      method: 'executeSnap',
+      params: [FAKE_SNAP_NAME, CODE, []],
+    });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      jsonrpc: '2.0',
+      id: 1,
+      result: 'OK',
+    });
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      id: 2,
+      method: 'snapRpc',
+      params: [
+        FAKE_SNAP_NAME,
+        FAKE_ORIGIN,
+        { jsonrpc: '2.0', method: '', params: [] },
+      ],
+    });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      id: 2,
+      jsonrpc: '2.0',
+      result: 'foo',
+    });
+
+    const testError = new Error('test error');
+    emitter.emit('unhandledrejection', testError);
+
+    expect(await executor.readCommand()).toStrictEqual({
+      jsonrpc: '2.0',
+      error: {
+        code: -32603,
+        data: {
+          stack: testError.stack,
+          snapName: 'local:foo',
+        },
+        message: testError.message,
+      },
+    });
+  });
+
+  it('notifies execution service of out of band errors via error', async () => {
+    const CODE = `
+    module.exports.onRpcRequest = async () => 'foo';
+    `;
+    const executor = new TestSnapExecutor();
+    const emitter = new EventEmitter();
+
+    jest
+      .spyOn(window, 'addEventListener')
+      .mockImplementation((type, listener) =>
+        emitter.on(type, listener as any),
+      );
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      id: 1,
+      method: 'executeSnap',
+      params: [FAKE_SNAP_NAME, CODE, []],
+    });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      jsonrpc: '2.0',
+      id: 1,
+      result: 'OK',
+    });
+
+    await executor.writeCommand({
+      jsonrpc: '2.0',
+      id: 2,
+      method: 'snapRpc',
+      params: [
+        FAKE_SNAP_NAME,
+        FAKE_ORIGIN,
+        { jsonrpc: '2.0', method: '', params: [] },
+      ],
+    });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      id: 2,
+      jsonrpc: '2.0',
+      result: 'foo',
+    });
+
+    const testError = new Error('test error');
+    emitter.emit('error', { error: testError });
+
+    expect(await executor.readCommand()).toStrictEqual({
+      jsonrpc: '2.0',
+      error: {
+        code: -32603,
+        data: {
+          stack: testError.stack,
+          snapName: 'local:foo',
+        },
+        message: testError.message,
+      },
+    });
+  });
+
   it('blocks Snaps from escaping confinement by using unbound this', async () => {
     const PAYLOAD = `
     console.error("Hack the planet");

diff --git a/packages/execution-environments/src/common/BaseSnapExecutor.ts b/packages/execution-environments/src/common/BaseSnapExecutor.ts
@@ -78,25 +78,18 @@ export class BaseSnapExecutor {
     );
   }
 
-  private errorHandler(
-    reason: string,
-    originalError: unknown,
-    data: Record<string, unknown>,
-  ) {
-    const error = new Error(reason);
-
-    const _originalError: Error | undefined = constructError(originalError);
-
-    const serializedError = serializeError(error, {
+  private errorHandler(error: unknown, data: Record<string, unknown>) {
+    const constructedError = constructError(error);
+    const serializedError = serializeError(constructedError, {
+      fallbackError,
       shouldIncludeStack: false,
     });
-
     this.notify({
       error: {
         ...serializedError,
         data: {
           ...data,
-          originalError: _originalError,
+          stack: constructedError?.stack,
         },
       },
     });
@@ -189,11 +182,11 @@ export class BaseSnapExecutor {
     }
 
     this.snapErrorHandler = (error: ErrorEvent) => {
-      this.errorHandler('Uncaught error in snap.', error.error, { snapName });
+      this.errorHandler(error.error, { snapName });
     };
 
     this.snapPromiseErrorHandler = (error: PromiseRejectionEvent) => {
-      this.errorHandler('Unhandled promise rejection in snap.', error.reason, {
+      this.errorHandler(error instanceof Error ? error : error.reason, {
         snapName,
       });
     };