fix: increase max crash key value length (#24782)

* fix: increase max crash key value length

* chore: fix linting

* chore: fix linux

* Update spec-main/api-crash-reporter-spec.ts

Co-authored-by: Jeremy Rose <[email protected]>

Co-authored-by: Samuel Attard <[email protected]>
Co-authored-by: Samuel Attard <[email protected]>

docs/api/crash-reporter.md

@@ -156,10 +156,16 @@ parameters in a renderer process will not result in those parameters being sent
 with crashes that occur in other renderer processes or in the main process.
 
 **Note:** Parameters have limits on the length of the keys and values. Key
-names must be no longer than 39 bytes, and values must be no longer than 127
+names must be no longer than 39 bytes, and values must be no longer than 20320
 bytes. Keys with names longer than the maximum will be silently ignored. Key
 values longer than the maximum length will be truncated.
 
+**Note:** On linux values that are longer than 127 bytes will be chunked into
+multiple keys, each 127 bytes in length.  E.g. `addExtraParameter('foo', 'a'.repeat(130))`
+will result in two chunked keys `foo__1` and `foo__2`, the first will contain
+the first 127 bytes and the second will contain the remaining 3 bytes.  On
+your crash reporting backend you should stitch together keys in this format.
+
 ### `crashReporter.removeExtraParameter(key)`
 
 * `key` String - Parameter key, must be no longer than 39 bytes.

shell/common/crash_keys.cc

@@ -24,7 +24,19 @@ namespace crash_keys {
 
 namespace {
 
-using ExtraCrashKeys = std::deque<crash_reporter::CrashKeyString<127>>;
+#if defined(OS_LINUX)
+// Breakpad has a flawed system of calculating the number of chunks
+// we add 127 bytes to force an extra chunk
+constexpr size_t kMaxCrashKeyValueSize = 20479;
+#else
+constexpr size_t kMaxCrashKeyValueSize = 20320;
+#endif
+
+static_assert(kMaxCrashKeyValueSize < crashpad::Annotation::kValueMaxSize,
+              "max crash key value length above what crashpad supports");
+
+using ExtraCrashKeys =
+    std::deque<crash_reporter::CrashKeyString<kMaxCrashKeyValueSize>>;
 ExtraCrashKeys& GetExtraCrashKeys() {
   static base::NoDestructor<ExtraCrashKeys> extra_keys;
   return *extra_keys;

spec-main/api-crash-reporter-spec.ts

@@ -196,19 +196,30 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_
   });
 
   ifdescribe(!isLinuxOnArm)('extra parameter limits', () => {
-    it('should truncate extra values longer than 127 characters', async () => {
+    function stitchLongCrashParam (crash: any, paramKey: string) {
+      if (crash[paramKey]) return crash[paramKey];
+      let chunk = 1;
+      let stitched = '';
+      while (crash[`${paramKey}__${chunk}`]) {
+        stitched += crash[`${paramKey}__${chunk}`];
+        chunk++;
+      }
+      return stitched;
+    }
+
+    it('should truncate extra values longer than 5 * 4096 characters', async () => {
       const { port, waitForCrash } = await startServer();
       const { remotely } = await startRemoteControlApp();
       remotely((port: number) => {
         require('electron').crashReporter.start({
           submitURL: `http://127.0.0.1:${port}`,
           ignoreSystemCrashHandler: true,
-          extra: { longParam: 'a'.repeat(130) }
+          extra: { longParam: 'a'.repeat(100000) }
         });
         setTimeout(() => process.crash());
       }, port);
       const crash = await waitForCrash();
-      expect(crash).to.have.property('longParam', 'a'.repeat(127));
+      expect(stitchLongCrashParam(crash, 'longParam')).to.have.lengthOf(160 * 127, 'crash should have truncated longParam');
     });
 
     it('should omit extra keys with names longer than the maximum', async () => {