[RecordReplay] Refine database seeding with error logging and comments [chromium/src : main]

[Friedrich Hauser (Gerrit)]

Attention needed from Adem Derinel

Friedrich Hauser added 1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Friedrich Hauser . resolved

Hi Adem,
Would you please take a look?

Open in Gerrit

Related details

Attention is currently required from:

• Adem Derinel

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ifee862444783af4f517dd8c038202452b5b15d0d

Gerrit-Change-Number: 7806643

Gerrit-PatchSet: 5

Gerrit-Owner: Friedrich Hauser <fried...@chromium.org>

Gerrit-Reviewer: Adem Derinel <der...@google.com>

Gerrit-Reviewer: Friedrich Hauser <fried...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: Andrew Rayskiy <green...@google.com>

Gerrit-CC: Anurag Simgeker <anurags...@google.com>

Gerrit-CC: Avi Drissman <a...@chromium.org>

Gerrit-CC: AyeAye Python Dispatcher <android-build-ayeay...@system.gserviceaccount.com>

Gerrit-CC: CJ DiMeglio <lethala...@chromium.org>

Gerrit-CC: Charles Hager <clh...@google.com>

Gerrit-CC: Christian Biesinger <cbies...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Chromium Metrics Reviews <chromium-met...@google.com>

Gerrit-CC: Daniel Cheng <dch...@chromium.org>

Gerrit-CC: Dirk Schulze <dsch...@chromium.org>

Gerrit-CC: Enterprise Policy Reviews <enterprise-p...@google.com>

Gerrit-CC: Frank Liberato <libe...@chromium.org>

Gerrit-CC: Fredrik Söderquist <f...@opera.com>

Gerrit-CC: Hans Wennborg <ha...@chromium.org>

Gerrit-CC: Henrique Ferreiro <hfer...@igalia.com>

Gerrit-CC: Heron Yang <hero...@google.com>

Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>

Gerrit-CC: Hongchan Choi <hong...@chromium.org>

Gerrit-CC: Hu, Ningxin <ningx...@intel.com>

Gerrit-CC: Ian Vollick <vol...@chromium.org>

Gerrit-CC: Ilya Biryukov <ibir...@google.com>

Gerrit-CC: James Maclean <wjma...@chromium.org>

Gerrit-CC: James Su <su...@chromium.org>

Gerrit-CC: Javier Fernandez <jfern...@igalia.com>

Gerrit-CC: Jerome Jiang <ji...@chromium.org>

Gerrit-CC: Jiewei Qian <q...@chromium.org>

Gerrit-CC: Kenneth Rohde Christiansen <kenneth.ch...@gmail.com>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Kevin McNee <mc...@chromium.org>

Gerrit-CC: Khushal Sagar <khusha...@chromium.org>

Gerrit-CC: Lei Zhang <the...@chromium.org>

Gerrit-CC: Linyu He <lin...@google.com>

Gerrit-CC: Luna Lu <loon...@chromium.org>

Gerrit-CC: Mangesh Ghiware <mghi...@google.com>

Gerrit-CC: Mark Schillaci <mschi...@google.com>

Gerrit-CC: Menard, Alexis <alexis...@intel.com>

Gerrit-CC: Michael Moss <mm...@chromium.org>

Gerrit-CC: Michael Wilson <mjwi...@chromium.org>

Gerrit-CC: Mirko Bonadei <mbon...@chromium.org>

Gerrit-CC: Moe Adel <ad...@google.com>

Gerrit-CC: Nate Chapin <jap...@chromium.org>

Gerrit-CC: Neil Coronado <ne...@google.com>

Gerrit-CC: Nektarios Paisios <nek...@chromium.org>

Gerrit-CC: Nico Weber <tha...@chromium.org>

Gerrit-CC: Nikhil Nayunigari <nikh...@google.com>

Gerrit-CC: Olga Gerchikov <gerc...@microsoft.com>

Gerrit-CC: Peter Beverloo <pe...@chromium.org>

Gerrit-CC: Peter Collingbourne <p...@chromium.org>

Gerrit-CC: Peter Williamson <pet...@chromium.org>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: Reid Kleckner <r...@chromium.org>

Gerrit-CC: Rijubrata Bhaumik <rijubrat...@intel.com>

Gerrit-CC: Ryan Sultanem <rs...@google.com>

Gerrit-CC: Sadrul Chowdhury <sad...@chromium.org>

Gerrit-CC: Sean Maher <sp...@chromium.org>

Gerrit-CC: Shu Yang <shu...@google.com>

Gerrit-CC: Simon Hangl <sim...@google.com>

Gerrit-CC: Sinan Sahin <sinan...@google.com>

Gerrit-CC: Stephen Chenney <sche...@chromium.org>

Gerrit-CC: Thiabaud Engelbrecht <thia...@google.com>

Gerrit-CC: Thorsten Kober <thor...@google.com>

Gerrit-CC: Tommy Li <tomm...@chromium.org>

Gerrit-CC: Xida Chen <xida...@chromium.org>

Gerrit-CC: Zewen Li <zew...@google.com>

Gerrit-CC: Zijie He <zij...@google.com>

Gerrit-CC: android-bu...@system.gserviceaccount.com <android-bu...@system.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-CC: srirama chandra sekhar <srir...@samsung.com>

Gerrit-Attention: Adem Derinel <der...@google.com>

Gerrit-Comment-Date: Mon, 04 May 2026 16:44:21 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Moe Adel (Gerrit)]

Attention needed from Adem Derinel and Friedrich Hauser

Moe Adel voted and added 4 comments

Votes added by Moe Adel

Code-Review

+1

4 comments

File components/record_replay/core/browser/capabilities_database.cc

Line 52, Patchset 5 (Latest): db_.set_error_callback(base::BindRepeating([](int error,

Moe Adel . unresolved

not sure, would like your input: What's the reason for using `NOTREACHED` compared to `(D)LOG` statements that do not crash?

Line 57, Patchset 5 (Latest): if ((error & 0xff) == 19) { // SQLITE_CONSTRAINT

Moe Adel . unresolved

does 0xff mean a constraint violation?

Line 60, Patchset 5 (Latest): NOTREACHED() << "CapabilitiesDatabase SQLite Error: " << error

Moe Adel . unresolved

I think use (D)LOG here to log runtime errors in the callback without causing a crash

Line 267, Patchset 5 (Latest): CHECK(false) << first_error;

Moe Adel . unresolved

I am wondering if we should loop in the finch team to add a check there that checks the string's integrity before it hits here. Is there a way to check that? I think a broken string will crash the browser for all users running that finch experiment. Maybe we should add a log statement here to keep track of that?

Open in Gerrit

Related details

Attention is currently required from:

• Adem Derinel
• Friedrich Hauser

Submit Requirements:

• Code-Coverage
• Code-Owners

• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ifee862444783af4f517dd8c038202452b5b15d0d

Gerrit-Change-Number: 7806643

Gerrit-PatchSet: 5

Gerrit-Owner: Friedrich Hauser <fried...@chromium.org>

Gerrit-Reviewer: Adem Derinel <der...@google.com>

Gerrit-Reviewer: Friedrich Hauser <fried...@chromium.org>

Gerrit-Reviewer: Moe Adel <ad...@google.com>

Gerrit-Attention: Friedrich Hauser <fried...@chromium.org>

Gerrit-Attention: Adem Derinel <der...@google.com>

Gerrit-Comment-Date: Mon, 04 May 2026 17:49:32 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Friedrich Hauser (Gerrit)]

Attention needed from Adem Derinel and Moe Adel

Friedrich Hauser added 5 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Friedrich Hauser . resolved

-CCs (sorry for the noise caused by uploading a bad rebase)

File components/record_replay/core/browser/capabilities_database.cc

Line 52, Patchset 5 (Latest): db_.set_error_callback(base::BindRepeating([](int error,

Moe Adel . unresolved

not sure, would like your input: What's the reason for using `NOTREACHED` compared to `(D)LOG` statements that do not crash?

Friedrich Hauser

This code is targeted at developers. Getting a crash was easier to interpret than looking for the reason of absent data. But agreed, I'll DLOG it since it barely affects the user's browsing experience to not have old recordings.

Line 57, Patchset 5 (Latest): if ((error & 0xff) == 19) { // SQLITE_CONSTRAINT

Moe Adel . unresolved

does 0xff mean a constraint violation?

Friedrich Hauser

& 0xff just gives the relevant bits for the error code — the entire error code would give me the extended error which I don't need.
19 is the error code for `SQLITE_CONSTRAINT` — so if foreign key or unique constraints would be broken ... I can also DLOG that to make it more clear.

Line 60, Patchset 5 (Latest): NOTREACHED() << "CapabilitiesDatabase SQLite Error: " << error

Moe Adel . unresolved

I think use (D)LOG here to log runtime errors in the callback without causing a crash

Friedrich Hauser

I'll use DLOG everywhere — I am always unsure when that's appropriate since the styleguide discourages all LOGs.

Line 267, Patchset 5 (Latest): CHECK(false) << first_error;

Moe Adel . unresolved

I am wondering if we should loop in the finch team to add a check there that checks the string's integrity before it hits here. Is there a way to check that? I think a broken string will crash the browser for all users running that finch experiment. Maybe we should add a log statement here to keep track of that?

Friedrich Hauser

There is no immediate plan to roll this out via Finch but I agree that the prospect of crashing every Chrome client on startup because of a typo is just a path that shouldn't exist. I'll make this a DumpWithoutCrashing.

Open in Gerrit

Related details

Attention is currently required from:

• Adem Derinel
• Moe Adel

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ifee862444783af4f517dd8c038202452b5b15d0d

Gerrit-Change-Number: 7806643

Gerrit-PatchSet: 5

Gerrit-Owner: Friedrich Hauser <fried...@chromium.org>

Gerrit-Reviewer: Adem Derinel <der...@google.com>

Gerrit-Reviewer: Friedrich Hauser <fried...@chromium.org>

Gerrit-Reviewer: Moe Adel <ad...@google.com>

Gerrit-Attention: Moe Adel <ad...@google.com>

Gerrit-Attention: Adem Derinel <der...@google.com>

Gerrit-Comment-Date: Tue, 05 May 2026 09:02:52 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Moe Adel <ad...@google.com>

[Friedrich Hauser (Gerrit)]

Attention needed from Adem Derinel

Friedrich Hauser has uploaded the change for review

Friedrich Hauser removed siyua+aut...@chromium.org, tbarzi...@chromium.org, thegreenf...@chromium.org, scheduler...@chromium.org, spang...@chromium.org, suetfei+wa...@google.com, torne...@chromium.org, tmartino+tran...@chromium.org, shuche...@chromium.org, trewin...@google.com, vakh+safe_br...@chromium.org, twifka...@chromium.org, security-...@chromium.org, thefro...@chromium.org, stanfie...@google.com, translat...@chromium.org, shend...@chromium.org, ukai+...@chromium.org, speed-metr...@chromium.org, tracing...@chromium.org, srahim...@chromium.org, storage...@chromium.org, sloboda...@chromium.org, tburkar...@chromium.org, subresource-f...@chromium.org, tgupta...@chromium.org, vaapi-...@chromium.org, sky+...@chromium.org, tommyw+w...@chromium.org, titoua...@chromium.org, tluk+...@chromium.org, shgar+aut...@google.com, servicewor...@chromium.org, siashah+au...@chromium.org, twelling...@chromium.org, shannc...@chromium.org, speed-metrics...@chromium.org, telemetr...@chromium.org, tranbaod...@chromium.org, toshikikikuchi+...@chromium.org, steimel+...@chromium.org, stevenjb+wa...@chromium.org, shimazu+se...@chromium.org and shimazu...@chromium.org from reviewers of this change.

Commit message

[RecordReplay] Refine database seeding with error logging and comments

Why this change is necessary:
- Fixes a database corruption hazard by changing the background task
  runner shutdown behavior to BLOCK_SHUTDOWN.
- Prevents silent database failures by adding an SQLite error callback.
- Adds extensive intent documentation across the seeding codebase.

Seeding formats supported:
1. Detailed Syntax: Seeding via a JSON file containing an explicit list
   of steps with individual descriptions and expected data keys.
2. Quick Syntax: Seeding via a JSON file containing top-level
   instructions or anchored messages only, which automatically
   synthesizes a single step (Step 1) to maintain schema uniformity.

Bug: 507806188

Change-Id: Ifee862444783af4f517dd8c038202452b5b15d0d

Change diff

Change information

Files:

• M components/record_replay/core/browser/BUILD.gn
• A components/record_replay/core/browser/annotation_parsing_utils.cc
• A components/record_replay/core/browser/annotation_parsing_utils.h
• M components/record_replay/core/browser/capabilities_database.cc
• M components/record_replay/core/browser/capabilities_database.h
• M components/record_replay/core/browser/file_activity_provider.cc
• M components/record_replay/core/browser/recording_data_manager_impl.cc
• M components/record_replay/core/browser/recording_data_manager_impl.h
• M components/record_replay/core/browser/recording_data_manager_impl_unittest.cc

Change size: L

Delta: 9 files changed, 425 insertions(+), 64 deletions(-)

Open in Gerrit

Related details

Attention is currently required from:

• Adem Derinel

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• No-Unresolved-Comments
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: newchange

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Ifee862444783af4f517dd8c038202452b5b15d0d

Gerrit-Change-Number: 7806643

Gerrit-PatchSet: 5

Gerrit-Owner: Friedrich Hauser <fried...@chromium.org>

Gerrit-Reviewer: Adem Derinel <der...@google.com>

Gerrit-Reviewer: Friedrich Hauser <fried...@chromium.org>

Gerrit-Reviewer: Moe Adel <ad...@google.com>

Gerrit-Attention: Adem Derinel <der...@google.com>

[Friedrich Hauser (Gerrit)]

Attention needed from Adem Derinel

Friedrich Hauser has uploaded the change for review

Friedrich Hauser removed pushi+watc...@google.com, wangdanny+watch-in...@google.com, schedule...@chromium.org, zackha...@chromium.org, vinnypersky+...@google.com, roagarw...@chromium.org, yhanada+...@chromium.org, yusufo...@chromium.org, rkgibso...@chromium.org, rsleev...@chromium.org, yuzo+...@chromium.org, zelin+watch-we...@chromium.org, xlythe+wa...@google.com, scheduler-...@chromium.org, vasilii+watchlis...@chromium.org, wychen...@chromium.org, yfriedm...@chromium.org, zol...@webkit.org, yongshun+...@google.com, rmcelra...@chromium.org, webap...@microsoft.com, roblia...@chromium.org, yyhyyh+fee...@google.com, webapks-...@chromium.org, pushi+wa...@google.com, wnwen...@chromium.org, ydago...@chromium.org, wangdanny+fe...@google.com, xiangdongkong+...@google.com, yhanada...@chromium.org, wfh+...@chromium.org, ramyagopa...@google.com, pushi+wat...@google.com, rainhar...@chromium.org, xiaochen...@chromium.org, xinghui...@chromium.org, webauthn...@chromium.org, rayanka...@chromium.org, yigu+...@chromium.org, rhalava...@chromium.org, yhanada+...@chromium.org, rsesek...@chromium.org, weiluanw...@google.com, video-networking...@google.com, yuezhang...@chromium.org, rrsilva+wat...@google.com, rginda...@chromium.org, yhanad...@chromium.org, yyhyyh+watch-inpu...@google.com and web-schedulin...@chromium.org from reviewers of this change.