Fix the cobalt stacktrace symbolization issue

[santoshmahto7]

Bug: 511316178

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive set of updates to the Cobalt repository, primarily focusing on lifecycle management via the new AppEventDelegate and AppEventRunner classes, dynamic crash annotations, optimized video memory tracking, and Evergreen updater Mojo interfaces. It also implements storage migration for C25 transitions and refines CPU and memory metrics collection. The code review identified several critical issues, including a potential iterator invalidation bug in base/callback_list.h during nested notifications, a possible NullPointerException in JniAndroid.java, redundant error handling in cobalt_new_checkout.py, a potential DCHECK crash in cobalt_cpu_metrics_emitter.cc from unclamped CPU usage, and an ineffective placement of _FILE_OFFSET_BITS in symbolize.cc.

I am having trouble creating individual review comments. Click here to see my feedback.

base/callback_list.h (244-257)

Calling CleanUpNullCallbacksPostIteration() at the end of every Notify() execution (including nested/re-entrant ones) can lead to iterator invalidation in the outer Notify() loop. If a callback is explicitly canceled or executed during a nested Notify(), it becomes null and is spliced into null_callbacks_ and potentially erased. If the outer Notify() loop has a saved next iterator pointing to this callback, that iterator will be invalidated, causing undefined behavior or a crash when the outer loop resumes.\n\nTo fix this, the cleanup of null callbacks should be deferred until the outermost Notify() completes (e.g., by using a re-entrancy counter instead of a simple boolean iterating_, and only performing the cleanup when the counter decrements to 0).

base/android/java/src/org/chromium/base/JniAndroid.java (54-72)

The constructor of UncaughtExceptionException and the helper method getTopFrameString do not perform null checks on uncaughtException. If uncaughtException is null, calling uncaughtException.getStackTrace() will throw a NullPointerException, crashing the exception handling flow itself.\n\nPlease add defensive null checks to handle a null uncaughtException gracefully.

        public UncaughtExceptionException(String nativeStackTrace, Throwable uncaughtException) {\n            super(\n                    (uncaughtException != null ? getTopFrameString(uncaughtException) : \"UnknownSource\") + System.lineSeparator()\n                            + \"Uncaught Java exception:\" + System.lineSeparator()\n                            + (uncaughtException != null ? Log.getStackTraceString(uncaughtException) : \"null\") + System.lineSeparator()\n                            + \"Native stack trace:\" + System.lineSeparator() + nativeStackTrace,\n                    uncaughtException);\n            if (uncaughtException != null) {\n                setStackTrace(uncaughtException.getStackTrace());\n            }\n        }\n\n        private static String getTopFrameString(Throwable uncaughtException) {\n            if (uncaughtException == null) {\n                return \"UnknownSource\";\n            }\n            StackTraceElement[] frames = uncaughtException.getStackTrace();\n            if (frames != null && frames.length > 0) {\n                StackTraceElement top = frames[0];\n                String fileName = top.getFileName();\n                return (fileName != null ? fileName : \"UnknownSource\") + \":\" + top.getLineNumber();\n            }\n            return \"UnknownSource\";\n        }

.agent/skills/cobalt-new-checkout/scripts/cobalt_new_checkout.py (357-382)

The error handling inside setup_remotes is redundant and potentially broken. If execute_command is called with check=False, it will not raise subprocess.CalledProcessError. However, if git remote set-url or git remote add fails inside the try block, it will raise CalledProcessError, which is caught by the except block. The except block then attempts to run git remote add fork ... again, which will fail with a 'remote already exists' error.\n\nWe can simplify this logic by checking the return code of git remote get-url fork and updating or adding the remote accordingly, without the redundant try-except block.

            try:\n                process_res = execute_command(\n                    [\"git\", \"remote\", \"get-url\", \"fork\"],\n                    cwd=self.repo_root,\n                    check=False)\n                if process_res.returncode == 0:\n                    current_remote = process_res.stdout.strip()\n                    if current_remote != fork_target_url:\n                        print(\"Updating `fork` URL remote configuration: \"\n                              f\"{current_remote} -> {fork_target_url}\")\n                        execute_command([\n                            \"git\", \"remote\", \"set-url\", \"fork\", fork_target_url\n                        ],\n                                        cwd=self.repo_root)\n                    else:\n                        print(\"Remote `fork` configured correctly.\")\n                else:\n                    execute_command(\n                        [\"git\", \"remote\", \"add\", \"fork\", fork_target_url\n                    ],\n                                        cwd=self.repo_root)\n            except subprocess.CalledProcessError as err:\n                print(f\"Failed to configure remote `fork`: {err}\", file=sys.stderr)\n                raise

cobalt/browser/metrics/cobalt_cpu_metrics_emitter.cc (46-52)

The calculated CPU percentage (cpu_usage / num_processors) can occasionally exceed 100% due to measurement interval discrepancies or system load spikes. Passing a value greater than 100 to base::UmaHistogramPercentage will trigger a DCHECK failure and crash debug builds.\n\nPlease clamp the calculated percentage to the range [0, 100] using std::clamp before logging it.

  const double cpu_usage = GetCpuUsage();\n  const int num_processors = base::SysInfo::NumberOfProcessors();\n  DCHECK_GT(num_processors, 0)\n      << \"Platform returned invalid number of processors.\";\n  // Divide by the number of processors to log average per core CPU usage.\n  int cpu_percentage = base::ClampRound<int>(cpu_usage / std::max(1, num_processors));\n  base::UmaHistogramPercentage(\n      \"CPU.Total.UsageInPercentage\",\n      std::clamp(cpu_percentage, 0, 100));

base/third_party/symbolize/symbolize.cc (55-56)

Defining _FILE_OFFSET_BITS after including GLOG_BUILD_CONFIG_INCLUDE might have no effect if that header already includes system headers like <unistd.h> or <sys/types.h>. To ensure _FILE_OFFSET_BITS 64 correctly enables 64-bit offsets for pread() and other system calls on 32-bit systems, it must be defined at the very top of the file before any #include statements.