From ddeaeb5ba51715b15e10e2631f2c92d3b8ee4a83 Mon Sep 17 00:00:00 2001
From: Mark Stacey <markjstacey@gmail.com>
Date: Fri, 18 Aug 2023 18:26:27 -0230
Subject: [PATCH] Fix Sentry breadcrumb collection during initialization
 (again) (#20532)

Sentry breadcrumb collection during initialization was broken in #20529
because we failed to consider that the `getSentryState` check was also
used for an opt-in check in the `beforeBreadcrumb` hook.

I had assumed that `getSentryState` was only used to get state to add
additional context to an error report. But the function has a second
purpose: to get state for the purposes of checking whether the user has
opted into MetaMetrics. In this second case, `mostRecentRetrievedState`
is sometimes unset (which violates an assumption made in #20529)

The `getMostRecentPersistedState` hook removed in #20529 has been
restored, ensuring that the `getSentryState` function returns Sentry
state after loading state for the first time, but before the first
error has occurred.

This mistake didn't cause e2e tests to fail because multiple errors are
currently thrown in the background upon initialization on `develop`
(relating to Snow scuttling). These errors were early enough that they
happened before the console logs that our breadcrumb test was testing
for. When #20529 was ported onto the v10.34.5 RC, these errors were not
present so the test failed correctly.
---
 app/scripts/background.js                    | 10 +++--
 app/scripts/lib/setup-initial-state-hooks.js | 40 ++++++++++++--------
 2 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/app/scripts/background.js b/app/scripts/background.js
index 830452033..44f9d177d 100644
--- a/app/scripts/background.js
+++ b/app/scripts/background.js
@@ -71,6 +71,12 @@ import DesktopManager from '@metamask/desktop/dist/desktop-manager';
 ///: END:ONLY_INCLUDE_IN
 /* eslint-enable import/order */
 
+// Setup global hook for improved Sentry state snapshots during initialization
+const inTest = process.env.IN_TEST;
+const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore();
+global.stateHooks.getMostRecentPersistedState = () =>
+  localStore.mostRecentRetrievedState;
+
 const { sentry } = global;
 const firstTimeState = { ...rawFirstTimeState };
 
@@ -93,10 +99,6 @@ let uiIsTriggering = false;
 const openMetamaskTabsIDs = {};
 const requestAccountTabIds = {};
 let controller;
-
-// state persistence
-const inTest = process.env.IN_TEST;
-const localStore = inTest ? new ReadOnlyNetworkStore() : new LocalStore();
 let versionedData;
 
 if (inTest || process.env.METAMASK_DEBUG) {
diff --git a/app/scripts/lib/setup-initial-state-hooks.js b/app/scripts/lib/setup-initial-state-hooks.js
index f65bb5257..9b3f18e09 100644
--- a/app/scripts/lib/setup-initial-state-hooks.js
+++ b/app/scripts/lib/setup-initial-state-hooks.js
@@ -28,9 +28,12 @@ const persistedStateMask = {
 };
 
 /**
- * Get a state snapshot to include with Sentry error reports. This uses the
- * persisted state pre-initialization, and the in-memory state post-
- * initialization. In both cases the state is anonymized.
+ * Get a state snapshot for Sentry. This is used to add additional context to
+ * error reports, and it's used when processing errors and breadcrumbs to
+ * determine whether the user has opted into Metametrics.
+ *
+ * This uses the persisted state pre-initialization, and the in-memory state
+ * post-initialization. In both cases the state is anonymized.
  *
  * @returns A Sentry state snapshot.
  */
@@ -47,22 +50,27 @@ globalThis.stateHooks.getSentryState = function () {
     };
   } else if (
     // This is truthy if Sentry has retrieved state at least once already. This
-    // should always be true because Sentry calls `getPersistedState` during
-    // error processing (before this function is called) if `getSentryAppState`
-    // hasn't been set yet.
-    sentryLocalStore.mostRecentRetrievedState
+    // should always be true when getting context for an error report, but can
+    // be unset when Sentry is performing the opt-in check.
+    sentryLocalStore.mostRecentRetrievedState ||
+    // This is only set in the background process.
+    globalThis.stateHooks.getMostRecentPersistedState
   ) {
-    return {
-      ...sentryState,
-      persistedState: maskObject(
-        sentryLocalStore.mostRecentRetrievedState,
-        persistedStateMask,
-      ),
-    };
+    const persistedState =
+      sentryLocalStore.mostRecentRetrievedState ||
+      globalThis.stateHooks.getMostRecentPersistedState();
+    // This can be unset when this method is called in the background for an
+    // opt-in check, but the state hasn't been loaded yet.
+    if (persistedState) {
+      return {
+        ...sentryState,
+        persistedState: maskObject(persistedState, persistedStateMask),
+      };
+    }
   }
   // This branch means that local storage has not yet been read, so we have
   // no choice but to omit the application state.
-  // This should be unreachable, unless an error was encountered during error
-  // processing.
+  // This should be unreachable when getting context for an error report, but
+  // can be false when Sentry is performing the opt-in check.
   return sentryState;
 };