feat(browser): simplify wrap/fill (#4286)

Author: mitsuhiko

packages/browser/src/helpers.ts

@@ -1,6 +1,14 @@
 import { captureException, getReportDialogEndpoint, withScope } from '@sentry/core';
 import { DsnLike, Event as SentryEvent, Mechanism, Scope, WrappedFunction } from '@sentry/types';
-import { addExceptionMechanism, addExceptionTypeValue, getGlobalObject, logger } from '@sentry/utils';
+import {
+  addExceptionMechanism,
+  addExceptionTypeValue,
+  addNonEnumerableProperty,
+  getGlobalObject,
+  getOriginalFunction,
+  logger,
+  markFunctionWrapped,
+} from '@sentry/utils';
 
 const global = getGlobalObject<Window>();
 let ignoreOnError: number = 0;
@@ -39,19 +47,28 @@ export function wrap(
   before?: WrappedFunction,
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
 ): any {
+  // for future readers what this does is wrap a function and then create
+  // a bi-directional wrapping between them.
+  //
+  // example: wrapped = wrap(original);
+  //  original.__sentry_wrapped__ -> wrapped
+  //  wrapped.__sentry_original__ -> original
+
   if (typeof fn !== 'function') {
     return fn;
   }
 
   try {
-    // We don't wanna wrap it twice
-    if (fn.__sentry__) {
-      return fn;
+    // if we're dealing with a function that was previously wrapped, return
+    // the original wrapper.
+    const wrapper = fn.__sentry_wrapped__;
+    if (wrapper) {
+      return wrapper;
     }
 
-    // If this has already been wrapped in the past, return that wrapped function
-    if (fn.__sentry_wrapped__) {
-      return fn.__sentry_wrapped__;
+    // We don't wanna wrap it twice
+    if (getOriginalFunction(fn)) {
+      return fn;
     }
   } catch (e) {
     // Just accessing custom props in some Selenium environments
@@ -73,14 +90,6 @@ export function wrap(
       // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
       const wrappedArguments = args.map((arg: any) => wrap(arg, options));
 
-      if (fn.handleEvent) {
-        // Attempt to invoke user-land function
-        // NOTE: If you are a Sentry user, and you are seeing this stack frame, it
-        //       means the sentry.javascript SDK caught an error invoking your application code. This
-        //       is expected behavior and NOT indicative of a bug with sentry.javascript.
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-        return fn.handleEvent.apply(this, wrappedArguments);
-      }
       // Attempt to invoke user-land function
       // NOTE: If you are a Sentry user, and you are seeing this stack frame, it
       //       means the sentry.javascript SDK caught an error invoking your application code. This
@@ -91,19 +100,17 @@ export function wrap(
 
       withScope((scope: Scope) => {
         scope.addEventProcessor((event: SentryEvent) => {
-          const processedEvent = { ...event };
-
           if (options.mechanism) {
-            addExceptionTypeValue(processedEvent, undefined, undefined);
-            addExceptionMechanism(processedEvent, options.mechanism);
+            addExceptionTypeValue(event, undefined, undefined);
+            addExceptionMechanism(event, options.mechanism);
           }
 
-          processedEvent.extra = {
-            ...processedEvent.extra,
+          event.extra = {
+            ...event.extra,
             arguments: args,
           };
 
-          return processedEvent;
+          return event;
         });
 
         captureException(ex);
@@ -124,26 +131,11 @@ export function wrap(
     }
   } catch (_oO) {} // eslint-disable-line no-empty
 
-  fn.prototype = fn.prototype || {};
-  sentryWrapped.prototype = fn.prototype;
-
-  Object.defineProperty(fn, '__sentry_wrapped__', {
-    enumerable: false,
-    value: sentryWrapped,
-  });
-
   // Signal that this function has been wrapped/filled already
   // for both debugging and to prevent it to being wrapped/filled twice
-  Object.defineProperties(sentryWrapped, {
-    __sentry__: {
-      enumerable: false,
-      value: true,
-    },
-    __sentry_original__: {
-      enumerable: false,
-      value: fn,
-    },
-  });
+  markFunctionWrapped(sentryWrapped, fn);
+
+  addNonEnumerableProperty(fn, '__sentry_wrapped__', sentryWrapped);
 
   // Restore original function name (not all browsers allow that)
   try {

packages/browser/src/integrations/trycatch.ts

@@ -1,5 +1,5 @@
 import { Integration, WrappedFunction } from '@sentry/types';
-import { fill, getFunctionName, getGlobalObject } from '@sentry/utils';
+import { fill, getFunctionName, getGlobalObject, getOriginalFunction } from '@sentry/utils';
 
 import { wrap } from '../helpers';
 
@@ -168,8 +168,9 @@ function _wrapXHR(originalSend: () => void): () => void {
           };
 
           // If Instrument integration has been called before TryCatch, get the name of original function
-          if (original.__sentry_original__) {
-            wrapOptions.mechanism.data.handler = getFunctionName(original.__sentry_original__);
+          const originalFunction = getOriginalFunction(original);
+          if (originalFunction) {
+            wrapOptions.mechanism.data.handler = getFunctionName(originalFunction);
           }
 
           // Otherwise wrap directly

packages/browser/test/unit/integrations/helpers.test.ts

@@ -31,23 +31,13 @@ describe('internal wrap()', () => {
 
   it('bail out with the original if accessing custom props go bad', () => {
     const fn = (() => 1337) as WrappedFunction;
-    fn.__sentry__ = false;
     Object.defineProperty(fn, '__sentry_wrapped__', {
       get(): void {
         throw new Error('boom');
       },
     });
 
     expect(wrap(fn)).toBe(fn);
-
-    Object.defineProperty(fn, '__sentry__', {
-      get(): void {
-        throw new Error('boom');
-      },
-      configurable: true,
-    });
-
-    expect(wrap(fn)).toBe(fn);
   });
 
   it('returns wrapped function if original was already wrapped', () => {
@@ -83,9 +73,6 @@ describe('internal wrap()', () => {
     expect(fn).toHaveProperty('__sentry_wrapped__');
     expect(fn.__sentry_wrapped__).toBe(wrapped);
 
-    expect(wrapped).toHaveProperty('__sentry__');
-    expect(wrapped.__sentry__).toBe(true);
-
     expect(wrapped).toHaveProperty('__sentry_original__');
     expect(wrapped.__sentry_original__).toBe(fn);
   });
@@ -128,27 +115,14 @@ describe('internal wrap()', () => {
     expect(fnArgB).toHaveProperty('__sentry_wrapped__');
   });
 
-  it('calls either `handleEvent` property if it exists or the original function', () => {
-    interface SinonEventSpy extends SinonSpy {
-      handleEvent: SinonSpy;
-    }
-
+  it('calls the original function', () => {
     const fn = spy();
-    const eventFn = spy() as SinonEventSpy;
-    eventFn.handleEvent = spy();
 
     wrap(fn)(123, 'Rick');
-    wrap(eventFn)(123, 'Morty');
 
     expect(fn.called).toBe(true);
     expect(fn.getCalls()[0].args[0]).toBe(123);
     expect(fn.getCalls()[0].args[1]).toBe('Rick');
-
-    expect(eventFn.handleEvent.called).toBe(true);
-    expect(eventFn.handleEvent.getCalls()[0].args[0]).toBe(123);
-    expect(eventFn.handleEvent.getCalls()[0].args[1]).toBe('Morty');
-
-    expect(eventFn.called).toBe(false);
   });
 
   it('preserves `this` context for all the calls', () => {
@@ -192,14 +166,11 @@ describe('internal wrap()', () => {
     const wrapped = wrap(fn);
 
     // Shouldn't show up in iteration
-    expect(Object.keys(fn)).toEqual(expect.not.arrayContaining(['__sentry__']));
     expect(Object.keys(fn)).toEqual(expect.not.arrayContaining(['__sentry_original__']));
     expect(Object.keys(fn)).toEqual(expect.not.arrayContaining(['__sentry_wrapped__']));
-    expect(Object.keys(wrapped)).toEqual(expect.not.arrayContaining(['__sentry__']));
     expect(Object.keys(wrapped)).toEqual(expect.not.arrayContaining(['__sentry_original__']));
     expect(Object.keys(wrapped)).toEqual(expect.not.arrayContaining(['__sentry_wrapped__']));
     // But should be accessible directly
-    expect(wrapped.__sentry__).toBe(true);
     expect(wrapped.__sentry_original__).toBe(fn);
     expect(fn.__sentry_wrapped__).toBe(wrapped);
   });

packages/core/src/integration.ts

@@ -1,6 +1,6 @@
 import { addGlobalEventProcessor, getCurrentHub } from '@sentry/hub';
 import { Integration, Options } from '@sentry/types';
-import { logger } from '@sentry/utils';
+import { addNonEnumerableProperty, logger } from '@sentry/utils';
 
 export const installedIntegrations: string[] = [];
 
@@ -77,6 +77,6 @@ export function setupIntegrations<O extends Options>(options: O): IntegrationInd
   // set the `initialized` flag so we don't run through the process again unecessarily; use `Object.defineProperty`
   // because by default it creates a property which is nonenumerable, which we want since `initialized` shouldn't be
   // considered a member of the index the way the actual integrations are
-  Object.defineProperty(integrations, 'initialized', { value: true });
+  addNonEnumerableProperty(integrations, 'initialized', true);
   return integrations;
 }

packages/core/src/integrations/functiontostring.ts

@@ -1,4 +1,5 @@
 import { Integration, WrappedFunction } from '@sentry/types';
+import { getOriginalFunction } from '@sentry/utils';
 
 let originalFunctionToString: () => void;
 
@@ -23,7 +24,7 @@ export class FunctionToString implements Integration {
 
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     Function.prototype.toString = function(this: WrappedFunction, ...args: any[]): string {
-      const context = this.__sentry_original__ || this;
+      const context = getOriginalFunction(this) || this;
       return originalFunctionToString.apply(context, args);
     };
   }

packages/types/src/wrappedfunction.ts

@@ -1,7 +1,6 @@
 /** JSDoc */
 export interface WrappedFunction extends Function {
   [key: string]: any;
-  __sentry__?: boolean;
   __sentry_wrapped__?: WrappedFunction;
   __sentry_original__?: WrappedFunction;
 }

packages/utils/src/misc.ts

@@ -2,6 +2,7 @@
 import { Event, Exception, Mechanism, StackFrame } from '@sentry/types';
 
 import { getGlobalObject } from './global';
+import { addNonEnumerableProperty } from './object';
 import { snipLine } from './string';
 
 /**
@@ -277,9 +278,7 @@ export function checkOrSetAlreadyCaught(exception: unknown): boolean {
   try {
     // set it this way rather than by assignment so that it's not ennumerable and therefore isn't recorded by the
     // `ExtraErrorData` integration
-    Object.defineProperty(exception, '__sentry_captured__', {
-      value: true,
-    });
+    addNonEnumerableProperty(exception, '__sentry_captured__', true);
   } catch (err) {
     // `exception` is a primitive, so we can't mark it seen
   }

packages/utils/src/object.ts

@@ -31,13 +31,7 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa
   // otherwise it'll throw "TypeError: Object.defineProperties called on non-object"
   if (typeof wrapped === 'function') {
     try {
-      wrapped.prototype = wrapped.prototype || {};
-      Object.defineProperties(wrapped, {
-        __sentry_original__: {
-          enumerable: false,
-          value: original,
-        },
-      });
+      markFunctionWrapped(wrapped, original);
     } catch (_Oo) {
       // This can throw if multiple fill happens on a global object like XMLHttpRequest
       // Fixes https://github.com/getsentry/sentry-javascript/issues/2043
@@ -47,6 +41,44 @@ export function fill(source: { [key: string]: any }, name: string, replacementFa
   source[name] = wrapped;
 }
 
+/**
+ * Defines a non enumerable property.  This creates a non enumerable property on an object.
+ *
+ * @param func The function to set a property to
+ * @param name the name of the special sentry property
+ * @param value the property to define
+ */
+// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
+export function addNonEnumerableProperty(func: any, name: string, value: any): void {
+  Object.defineProperty(func, name, {
+    value: value,
+  });
+}
+
+/**
+ * Remembers the original function on the wrapped function and
+ * patches up the prototype.
+ *
+ * @param wrapped the wrapper function
+ * @param original the original function that gets wrapped
+ */
+export function markFunctionWrapped(wrapped: WrappedFunction, original: WrappedFunction): void {
+  const proto = original.prototype || {};
+  wrapped.prototype = original.prototype = proto;
+  addNonEnumerableProperty(wrapped, '__sentry_original__', original);
+}
+
+/**
+ * This extracts the original function if available.  See
+ * `markFunctionWrapped` for more information.
+ *
+ * @param func the function to unwrap
+ * @returns the unwrapped version of the function if available.
+ */
+export function getOriginalFunction(func: WrappedFunction): WrappedFunction | undefined {
+  return func.__sentry_original__;
+}
+
 /**
  * Encodes given object into url-friendly format
  *