Issack John
← Writing

July 24, 2026 · Chromium · Networking · Reporting API

Fixing a Reporting API Race in Chromium

Chromium's crash reporting browser tests were disabled, with comments above them pointing at two flakiness bugs, one for Mac and one for every other platform.

I enabled them once in April 2024 by loosening an assertion. That was reverted three days later. The second attempt, a year on, found an ordering bug in the Reporting API itself.

The First Attempt

These tests drive the crash reporting path end to end. A page opts in, the test makes the renderer unresponsive, the browser kills the process with RESULT_CODE_HUNG, and the test asserts on the resulting report: its URL, a reason of "unresponsive", and for the opted-in case a JavaScript call stack.

CL 5422052 addressed a race between two of those steps. Collecting the call stack needs a round trip to the renderer, and the shutdown could finish before the response came back. The change accepted either the real stack or a placeholder: 

// The process may be shutdown before we receive a response from the
// renderer with the call stack.
EXPECT_TRUE(call_stack->find("infiniteLoop") != std::string::npos ||
            *call_stack == "Unable to collect JS call stack.");

That let the tests run, at the cost of allowing a passing run in which the call stack was never collected. It did not hold. The change was reverted three days later after failures on Win10 Tests x64, and the tests went back to being disabled.

The Race

I found it by adding logging along the path a report takes through the service, and watching one get dropped rather than delivered. The problem was not in the test. A report could be dropped between being queued and being stored, which means a site that had opted into crash reports would not receive one for a hang that should have produced it. The sequence:

  1. A report is queued with a reporting source.
  2. SendReportsAndRemoveSource() is called for that source, because the document that produced it is gone.
  3. No reports are found to deliver, because the report has not been added to the cache yet.
  4. The source is marked as expired.
  5. The queued report reaches ReportingCacheImpl::AddReport(), which drops it, because its source is now expired.

The drop in step 5 is deliberate:

// Drop the report if its reporting source is already marked as expired.
// This should only happen in testing as reporting source is only marked
// expiring when the document that can generate report is gone.
if (reporting_source.has_value() &&
    expired_sources_.find(reporting_source.value()) !=
        expired_sources_.end()) {
  return;
}

The reasoning in that comment holds as long as the two operations run in the order they were requested. They did not.

The Fix

ReportingServiceImpl defers work while it is still loading. Anything routed through DoOrBacklogTask() either runs immediately or goes onto a backlog to be replayed once initialization completes:

void DoOrBacklogTask(base::OnceClosure task) {
  if (shut_down_)
    return;

  FetchAllClientsFromStoreIfNecessary();

  if (!initialized_) {
    task_backlog_.push_back(std::move(task));
    return;
  }

  std::move(task).Run();
}

QueueReport() went through that path. SendReportsAndRemoveSource() did not, and instead called into the delivery agent and the cache directly. While the service was still loading, queueing a report was deferred and expiring its source was not, which produces exactly the inversion above.

Routing the expiration through the same queue is the whole fix:

void SendReportsAndRemoveSource(
    const base::UnguessableToken& reporting_source) override {
  DCHECK(!reporting_source.is_empty());
  // Queue expiration of the reporting sources as backlog tasks if the
  // reporting service is not yet initialized, so that reports and expirations
  // remain well-ordered.
  DoOrBacklogTask(
      base::BindOnce(&ReportingServiceImpl::DoSendReportsAndRemoveSource,
                     base::Unretained(this), reporting_source));
}

void DoSendReportsAndRemoveSource(
    const base::UnguessableToken& reporting_source) {
  context_->delivery_agent()->SendReportsForSource(reporting_source);
  context_->cache()->SetExpiredSource(reporting_source);
}

Both operations now pass through one ordered path, so a report queued before a source expiration is processed before it.

The backlog is only used before the service finishes initializing, which only happens with a persistent reporting store, because that store loads asynchronously. The window is therefore narrow: a renderer has to become unresponsive while the store is still loading. It was wide enough to keep a test suite disabled for a year.

Testing It

A unit test drives the sequence directly instead of trying to hit it by timing:

TEST_P(ReportingServiceTest,
       ProcessReportsBeforeSourceExpirationWhenUninitialized) {
  // Test only relevant when using a persistent store. The store requires async
  // loading, creating the opportunity for backlog tasks to accumulate before
  // initialization completes.
  if (!store()) {
    GTEST_SKIP();
  }
  // ... set up document reporting endpoints ...

  // Add a "create report" task to the backlog (service not initialized yet).
  service()->QueueReport(kUrl_, kReportingSource_, kNak_, kUserAgent_, kGroup_,
                         kType_, base::Value::Dict(), 0,
                         ReportingTargetType::kDeveloper);

  // Now simulate the source being destroyed, adding a "mark source as
  // expired" task to the backlog.
  service()->SendReportsAndRemoveSource(*kReportingSource_);

  // Verify neither operation has been processed yet.
  // ...

  // Now finish loading, which should process the backlog.
  FinishLoading(true /* load_success */);

  // 1. First the "create report" task was processed
  context()->cache()->GetReports(&reports);
  EXPECT_EQ(1u, reports.size());
  // 2. Then the "mark source as expired" task was processed
  EXPECT_TRUE(
      context()->cache()->GetExpiredSources().contains(*kReportingSource_));
}

The browser tests needed a separate change. They were marking the renderer unresponsive, calling Shutdown(RESULT_CODE_HUNG), and continuing without waiting for the process to actually exit. That got replaced with a helper in content/public/test/browser_test_utils.cc that waits: 

void SimulateUnresponsivePrimaryMainFrameAndWaitForExit(
    WebContents* web_contents) {
  RenderProcessHost* rph = web_contents->GetPrimaryMainFrame()->GetProcess();
  RenderProcessHostWatcher watcher(
      rph, RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);

  SimulateUnresponsiveRenderer(
      web_contents, web_contents->GetPrimaryMainFrame()->GetRenderWidgetHost());

  EXPECT_TRUE(rph->Shutdown(RESULT_CODE_HUNG));
  watcher.Wait();
  EXPECT_FALSE(watcher.did_exit_normally());
  EXPECT_TRUE(web_contents->IsCrashed());
}

Four call sites in the reporting browser tests now share it, and the DISABLED_ prefixes came off.

Conclusion

The fix landed on April 4, 2025, a year after the revert.

One gap is left. The call stack assertion only runs when a call stack is present, so that part of the coverage is still weaker than the rest. It is tracked in crbug.com/407473725.

References

← Back to writing