fix: Crashes due to shadowed global scope object

Changes instrumentation to prevent crashes in environments/code where global
or globalThis objects are shadowed or manipulated.

Author: edisigci

src/config.ts

@@ -30,6 +30,12 @@ export class Config {
   // If it's 0 then no async tracking.
   public readonly asyncTrackingTimeout: number;
 
+  // This flag controls whether a check is generated for the existence
+  // of the AppMap record function in the global namespace. The check prevents
+  // crashes in environments where the global/globalThis object is shadowed or
+  // manipulated. This flag allows easy toggling of the check during testing.
+  public readonly generateGlobalRecordHookCheck: boolean = true;
+
   private readonly document?: Document;
   private migrationPending = false;
 

src/hooks/__tests__/instrument.test.ts

@@ -52,8 +52,11 @@ describe(instrument.transform, () => {
           "static": true
         }];
 
+        ${instrument.__appmapRecordInitFunctionDeclarationCode}
+        ${instrument.__appmapRecordVariableDeclarationCode}
+
         function testFun(arg) {
-          return global.AppMapRecordHook.call(this, () => {
+          return __appmapRecord.call(this, () => {
             return arg + 1;
           }, arguments, __appmapFunctionRegistry[0]);
         }
@@ -92,9 +95,12 @@ describe(instrument.transform, () => {
           }
         }];
 
+        ${instrument.__appmapRecordInitFunctionDeclarationCode}
+        ${instrument.__appmapRecordVariableDeclarationCode}
+
         class TestClass {
           foo(value) {
-            return global.AppMapRecordHook.call(this, () => {
+            return __appmapRecord.call(this, () => {
               return value + 1;
             }, arguments, __appmapFunctionRegistry[0]);
           }
@@ -175,21 +181,24 @@ describe(instrument.transform, () => {
         },
       ];
 
-      const outer = (...$appmap$args) => global.AppMapRecordHook.call(
+      ${instrument.__appmapRecordInitFunctionDeclarationCode}
+      ${instrument.__appmapRecordVariableDeclarationCode}
+
+      const outer = (...$appmap$args) => __appmapRecord.call(
         undefined,
         (arg) => arg + 42,
         $appmap$args,
         __appmapFunctionRegistry[0],
       );
 
       export const testFun = (...$appmap$args) =>
-        global.AppMapRecordHook.call(
+        __appmapRecord.call(
           undefined,
           (arg) => {
             var s42 = y => y - 42;
             let s43 = y => y - 43;
             const inner = (...$appmap$args) =>
-              global.AppMapRecordHook.call(
+              __appmapRecord.call(
                 undefined,
                 (x) => s42(x) * 2,
                 $appmap$args,
@@ -255,12 +264,15 @@ describe(instrument.transform, () => {
         },
       ];
 
+      ${instrument.__appmapRecordInitFunctionDeclarationCode}
+      ${instrument.__appmapRecordVariableDeclarationCode}
+
       exports.testFun = (...$appmap$args) =>
-        global.AppMapRecordHook.call(
+        __appmapRecord.call(
           undefined,
           (arg) => {
             const inner = (...$appmap$args) =>
-              global.AppMapRecordHook.call(
+              __appmapRecord.call(
                 undefined,
                 (x) => x * 2,
                 $appmap$args,
@@ -303,8 +315,11 @@ describe(instrument.transform, () => {
           "klassOrFile": "test",
           "static": true
         }];
-
-        const testFun = (...$appmap$args) => global.AppMapRecordHook.call(undefined, x => x * 2,
+        
+        ${instrument.__appmapRecordInitFunctionDeclarationCode}
+        ${instrument.__appmapRecordVariableDeclarationCode}
+        
+        const testFun = (...$appmap$args) => __appmapRecord.call(undefined, x => x * 2,
           $appmap$args, __appmapFunctionRegistry[0]);
 
         exports.testFun = testFun;

src/hooks/instrument.ts

@@ -176,6 +176,12 @@ export function transform(
 
   if (transformedFunctionInfos.length === 0) return program;
 
+  // Add these to prevent crash from shadowing of "global" and/or "globalThis".
+  if (config().generateGlobalRecordHookCheck) {
+    program.body.unshift(__appmapRecordVariableDeclaration);
+    program.body.unshift(__appmapRecordInitFunctionDeclaration);
+  }
+
   // Add a global variable to hold the function registry (see recorder.ts)
   const functionRegistryAssignment: ESTree.VariableDeclaration = {
     type: "VariableDeclaration",
@@ -253,12 +259,12 @@ function wrapCallable(
 ): ESTree.CallExpression {
   // (1) Pass the function as an arrow function expression even if the original
   // function is not an arrow function, because of a potential super keyword inside.
-  //    global.AppMapRecordHook.call(this|undefined, () => {...}, arguments, __appmapFunctionRegistry[i])
+  //    __appmapRecord.call(this|undefined, () => {...}, arguments, __appmapFunctionRegistry[i])
   //
   // (2) If it's a generator function then pass it as a generator function since
   // yield keyword cannot be used inside an arrow function. We don't care about (1)
   // since we don't transform generator methods due to the potential super keyword inside.
-  //    yield* global.AppMapRecordHook.call(this|undefined, function* f() {...}, arguments, __appmapFunctionRegistry[i])
+  //    yield* __appmapRecord.call(this|undefined, function* f() {...}, arguments, __appmapFunctionRegistry[i])
 
   let functionArgument: ESTree.Expression =
     fd.type === "ArrowFunctionExpression"
@@ -279,7 +285,9 @@ function wrapCallable(
   if (fd.type != "ArrowFunctionExpression") functionArgument = { ...functionArgument, params: [] };
 
   return call_(
-    memberId("global", "AppMapRecordHook", "call"),
+    config().generateGlobalRecordHookCheck
+      ? memberId("__appmapRecord", "call")
+      : memberId("global", "AppMapRecordHook", "call"),
     thisArg,
     functionArgument,
     argsArg,
@@ -352,3 +360,27 @@ function exportName(expr: ESTree.Expression): ESTree.Identifier | undefined {
   }
   return undefined;
 }
+
+export const __appmapRecordVariableDeclarationCode = `const __appmapRecord = __appmapRecordInit()`;
+const __appmapRecordVariableDeclaration = parse(__appmapRecordVariableDeclarationCode, {
+  module: true,
+}).body[0] as ESTree.VariableDeclaration;
+
+export const __appmapRecordInitFunctionDeclarationCode = `
+  function __appmapRecordInit() {
+    let g = null;
+    try {
+      g = global.AppMapRecordHook;
+    } catch (e) {
+      try {
+        g = globalThis.AppMapRecordHook;
+      } catch (e) {}
+      // If global/globalThis is shadowed in the top level, we'll get:
+      // ReferenceError: Cannot access 'global' before initialization    
+    }
+    // Bypass recording if we can't access recorder to prevent a crash.
+    return g ?? ((fun, argsArg) => fun.apply(this, argsArg));
+  }`;
+const __appmapRecordInitFunctionDeclaration = parse(__appmapRecordInitFunctionDeclarationCode, {
+  module: true,
+}).body[0] as ESTree.FunctionDeclaration;

test/__snapshots__/simple.test.ts.snap

@@ -828,6 +828,81 @@ exports[`mapping a script with import attributes/assertions 1`] = `
 }
 `;
 
+exports[`mapping a script with shadowed global object 1`] = `
+{
+  "classMap": [
+    {
+      "children": [
+        {
+          "children": [
+            {
+              "location": "global.js:3",
+              "name": "shadow",
+              "static": true,
+              "type": "function",
+            },
+          ],
+          "name": "global",
+          "type": "class",
+        },
+      ],
+      "name": "simple",
+      "type": "package",
+    },
+  ],
+  "events": [
+    {
+      "defined_class": "global",
+      "event": "call",
+      "id": 1,
+      "lineno": 3,
+      "method_id": "shadow",
+      "parameters": [
+        {
+          "class": "String",
+          "name": "global",
+          "value": "'Hello'",
+        },
+        {
+          "class": "String",
+          "name": "globalThis",
+          "value": "'World'",
+        },
+      ],
+      "path": "global.js",
+      "static": true,
+      "thread_id": 0,
+    },
+    {
+      "elapsed": 31.337,
+      "event": "return",
+      "id": 2,
+      "parent_id": 1,
+      "thread_id": 0,
+    },
+  ],
+  "metadata": {
+    "app": "simple",
+    "client": {
+      "name": "appmap-node",
+      "url": "https://github.com/getappmap/appmap-node",
+      "version": "test node-appmap version",
+    },
+    "language": {
+      "engine": "Node.js",
+      "name": "javascript",
+      "version": "test node version",
+    },
+    "name": "test process recording",
+    "recorder": {
+      "name": "process",
+      "type": "process",
+    },
+  },
+  "version": "1.12",
+}
+`;
+
 exports[`mapping a script with tangled async functions 1`] = `
 {
   "classMap": [

test/simple.test.ts

@@ -87,6 +87,11 @@ integrationTest("mapping a custom Error class with a message property", () => {
   expect(readAppmap()).toMatchSnapshot();
 });
 
+integrationTest("mapping a script with shadowed global object", () => {
+  expect(runAppmapNode("global.js").status).toBe(0);
+  expect(readAppmap()).toMatchSnapshot();
+});
+
 integrationTestSkipOnWindows("finish signal is handled", async () => {
   const server = spawnAppmapNode("server.mjs");
   await new Promise<void>((r) =>

test/simple/global.js

@@ -0,0 +1,7 @@
+const global = "Hello";
+
+function shadow(global, globalThis) {
+  console.log(global, globalThis);
+}
+
+shadow(global, "World");