Alex Gough (Gerrit)

unread,

Jul 9, 2024, 1:08:56 PM (8 days ago) Jul 9

to Alex Gough, Calder Kitagawa, Theresa Sullivan, chromium...@chromium.org, devtools...@chromium.org, Andrew Rayskiy, Nancy Wang, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, Lei Zhang, Tommy Li, alancutter...@chromium.org, andysjl...@chromium.org, arc-review...@google.com, asvitki...@chromium.org, blundell+...@chromium.org, chfreme...@chromium.org, chlily...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dibyapal+wa...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, dominickn+wat...@chromium.org, dominickn+...@chromium.org, dominickn+watch-...@chromium.org, dpr-eng+c...@google.com, druber...@chromium.org, dtraino...@chromium.org, ericwillige...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, kinuko+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mgiuca...@chromium.org, mxcai+watch...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, oshima...@chromium.org, performance-m...@chromium.org, philli...@chromium.org, pmonett...@chromium.org, rginda...@chromium.org, steimel+...@chromium.org, tsergea...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, yhanada+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org

Attention needed from Calder Kitagawa and Theresa Sullivan

Message from Alex Gough

Hi everyone - this is generated with `git cl split --max_depth=1`.

Apologies if you received an earlier not-working version of this
change - a rebase confused my tool - I've reviewed the whole
diff pre-split and think the same problem hasn't recurred.

I've run various speedometer runs on this and have not seen
any measurable changes - the search used to prime this change
shouldn't have picked any expensive-seeming calculations as
it just looks for a variable being compared to an .end().

Open in Gerrit

Related details

Attention is currently required from:

Calder Kitagawa
Theresa Sullivan

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Theresa Sullivan (Gerrit)

unread,

Jul 9, 2024, 1:26:18 PM (8 days ago) Jul 9

to Alex Gough, Calder Kitagawa, chromium...@chromium.org, devtools...@chromium.org, Andrew Rayskiy, Nancy Wang, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, Lei Zhang, Tommy Li, alancutter...@chromium.org, andysjl...@chromium.org, arc-review...@google.com, asvitki...@chromium.org, blundell+...@chromium.org, chfreme...@chromium.org, chlily...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dibyapal+wa...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, dominickn+wat...@chromium.org, dominickn+...@chromium.org, dominickn+watch-...@chromium.org, dpr-eng+c...@google.com, druber...@chromium.org, dtraino...@chromium.org, ericwillige...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, kinuko+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mgiuca...@chromium.org, mxcai+watch...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, oshima...@chromium.org, performance-m...@chromium.org, philli...@chromium.org, pmonett...@chromium.org, rginda...@chromium.org, steimel+...@chromium.org, tsergea...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, yhanada+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org

Attention needed from Alex Gough and Calder Kitagawa

Theresa Sullivan voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Alex Gough
Calder Kitagawa

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Calder Kitagawa (Gerrit)

unread,

Jul 9, 2024, 1:30:07 PM (8 days ago) Jul 9

to Alex Gough, Theresa Sullivan, chromium...@chromium.org, devtools...@chromium.org, Andrew Rayskiy, Nancy Wang, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, Lei Zhang, Tommy Li, alancutter...@chromium.org, andysjl...@chromium.org, arc-review...@google.com, asvitki...@chromium.org, blundell+...@chromium.org, chfreme...@chromium.org, chlily...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dibyapal+wa...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, dominickn+wat...@chromium.org, dominickn+...@chromium.org, dominickn+watch-...@chromium.org, dpr-eng+c...@google.com, druber...@chromium.org, dtraino...@chromium.org, ericwillige...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, kinuko+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mgiuca...@chromium.org, mxcai+watch...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, oshima...@chromium.org, performance-m...@chromium.org, philli...@chromium.org, pmonett...@chromium.org, rginda...@chromium.org, steimel+...@chromium.org, tsergea...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, yhanada+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org

Attention needed from Alex Gough

Calder Kitagawa voted and added 1 comment

Votes added by Calder Kitagawa

Code-Review	+1
Commit-Queue	+2

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Calder Kitagawa . resolved

LGTM

Open in Gerrit

Related details

Attention is currently required from:

Alex Gough

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Jul 9, 2024, 2:15:40 PM (8 days ago) Jul 9

to Alex Gough, Chromium LUCI CQ, Calder Kitagawa, Theresa Sullivan, chromium...@chromium.org, devtools...@chromium.org, Andrew Rayskiy, Nancy Wang, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, Lei Zhang, Tommy Li, alancutter...@chromium.org, andysjl...@chromium.org, arc-review...@google.com, asvitki...@chromium.org, blundell+...@chromium.org, chfreme...@chromium.org, chlily...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dibyapal+wa...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, dominickn+wat...@chromium.org, dominickn+...@chromium.org, dominickn+watch-...@chromium.org, dpr-eng+c...@google.com, druber...@chromium.org, dtraino...@chromium.org, ericwillige...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, kinuko+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mgiuca...@chromium.org, mxcai+watch...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, oshima...@chromium.org, performance-m...@chromium.org, philli...@chromium.org, pmonett...@chromium.org, rginda...@chromium.org, steimel+...@chromium.org, tsergea...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, yhanada+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org

Attention needed from Alex Gough

findit...@appspot.gserviceaccount.com voted Code-Coverage+1

This change meets the code coverage requirements.

Code-Coverage

+1

Open in Gerrit

Related details

Attention is currently required from:

Alex Gough

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jul 9, 2024, 2:42:21 PM (8 days ago) Jul 9

to Alex Gough, findit...@appspot.gserviceaccount.com, Calder Kitagawa, Theresa Sullivan, chromium...@chromium.org, devtools...@chromium.org, Andrew Rayskiy, Nancy Wang, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, Lei Zhang, Tommy Li, alancutter...@chromium.org, andysjl...@chromium.org, arc-review...@google.com, asvitki...@chromium.org, blundell+...@chromium.org, chfreme...@chromium.org, chlily...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dibyapal+wa...@chromium.org, dmurph+wat...@chromium.org, dmurph+watc...@chromium.org, dominickn+wat...@chromium.org, dominickn+...@chromium.org, dominickn+watch-...@chromium.org, dpr-eng+c...@google.com, druber...@chromium.org, dtraino...@chromium.org, ericwillige...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, grt+...@chromium.org, hidehik...@chromium.org, kinuko+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mattreyno...@chromium.org, mgiuca...@chromium.org, mxcai+watch...@chromium.org, nwoked...@chromium.org, odejesu...@chromium.org, oshima...@chromium.org, performance-m...@chromium.org, philli...@chromium.org, pmonett...@chromium.org, rginda...@chromium.org, steimel+...@chromium.org, tsergea...@chromium.org, vakh+safe_br...@chromium.org, webap...@microsoft.com, xinghui...@chromium.org, yhanada+...@chromium.org, zackha...@chromium.org, zelin+watch-we...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Turn iterator $var!=.end() DCHECK() into CHECK() for chrome

Accessing an invalid iterator can sometimes be a security
issue and these checks are cheap, so upgrade to CHECKs.

Added checks are NotFatalUntil::M130.

`base/not_fatal_until.h` is added using tools/add_header.py,
this may result in some main-file (foo.h for foo.cc) headers
being re-sorted to be first as part `git cl format` of this CL.

For this CL instances were located with a weggli query:

```
weggli --verbose=1 --cpp \
      'DCHECK($var != _.end());' \
   -p 'DCHECK(_.end() != $var);' \
   -p 'DCHECK_NE($var, _.end());' \
   -p 'DCHECK_NE(_.end(), $var);'
```

which should avoid any potentially expensive calculations
of the thing to be matched against .end().

This CL was uploaded by git cl split.

R=ckit...@chromium.org, twell...@chromium.org

Bug: 351745839

Change-Id: I47a73d27caa2ae0ca9218e65d5af856dc7fc7fde

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5689572

Code-Coverage: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Commit-Queue: Calder Kitagawa <ckit...@chromium.org>

Reviewed-by: Calder Kitagawa <ckit...@chromium.org>

Reviewed-by: Theresa Sullivan <twell...@chromium.org>

Auto-Submit: Alex Gough <aj...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1325013}

Files:

M chrome/browser/android/compositor/tab_content_manager.cc
M chrome/browser/android/historical_tab_saver.cc
M chrome/browser/apps/app_service/app_icon/app_icon_reader.cc
M chrome/browser/apps/app_service/app_notifications.cc
M chrome/browser/apps/app_service/app_service_proxy_ash.cc
M chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc
M chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc
M chrome/browser/devtools/device/devtools_android_bridge.cc
M chrome/browser/devtools/devtools_ui_bindings.cc
M chrome/browser/download/notification/multi_profile_download_notifier.cc
M chrome/browser/enterprise/connectors/analysis/files_request_handler.cc
M chrome/browser/extensions/api/web_authentication_proxy/web_authentication_proxy_api.cc
M chrome/browser/extensions/chrome_app_icon_service.cc
M chrome/browser/extensions/device_permissions_dialog_controller.cc
M chrome/browser/extensions/extension_assets_manager_chromeos.cc
M chrome/browser/extensions/global_shortcut_listener.cc
M chrome/browser/extensions/global_shortcut_listener_win.cc
M chrome/browser/intranet_redirect_detector.cc
M chrome/browser/media/webrtc/webrtc_event_log_manager_remote.cc
M chrome/browser/media_galleries/chromeos/mtp_device_delegate_impl_chromeos.cc
M chrome/browser/media_galleries/fileapi/mtp_device_map_service.cc
M chrome/browser/metrics/power/process_monitor.cc
M chrome/browser/notifications/notification_channels_provider_android_unittest.cc
M chrome/browser/notifications/scheduler/internal/display_decider.cc
M chrome/browser/performance_manager/user_tuning/profile_discard_opt_out_list_helper.cc
M chrome/browser/predictors/preconnect_manager.cc
M chrome/browser/predictors/prefetch_manager.cc
M chrome/browser/resource_coordinator/session_restore_policy.cc
M chrome/browser/resource_coordinator/tab_load_tracker.cc
M chrome/browser/safe_browsing/download_protection/download_protection_service.cc
M chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc
M chrome/browser/sessions/tab_loader.cc
M chrome/browser/signin/chrome_signin_proxying_url_loader_factory.cc
M chrome/browser/task_manager/mock_web_contents_task_manager.cc
M chrome/browser/task_manager/providers/worker_task_provider.cc
M chrome/browser/task_manager/sampling/task_manager_impl.cc
M chrome/browser/trusted_vault/trusted_vault_client_android.cc
M chrome/browser/ui/android/device_dialog/usb_chooser_dialog_android.cc
M chrome/browser/ui/thumbnails/thumbnail_image.cc
M chrome/browser/ui/thumbnails/thumbnail_scheduler_impl.cc
M chrome/browser/ui/views/download/bubble/download_bubble_row_view.cc
M chrome/browser/ui/views/global_media_controls/media_item_ui_device_selector_view.cc
M chrome/browser/ui/webui/discards/graph_dump_impl.cc
M chrome/browser/usb/usb_chooser_controller.cc
M chrome/browser/web_applications/externally_managed_app_manager_impl_unittest.cc
M chrome/browser/web_applications/web_app_sync_bridge.cc
M chrome/browser/win/conflicts/incompatible_applications_updater.cc
M chrome/browser/win/conflicts/module_blocklist_cache_updater.cc
M chrome/test/base/testing_profile_manager.cc

Change size: M

Delta: 49 files changed, 115 insertions(+), 65 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Theresa Sullivan, +1 by Calder Kitagawa

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Turn iterator $var!=.end() DCHECK() into CHECK() for chrome [chromium/src : main]

Alex Gough (Gerrit)

Message from Alex Gough

Related details

Theresa Sullivan (Gerrit)

Theresa Sullivan voted Code-Review+1

Related details

Calder Kitagawa (Gerrit)

Calder Kitagawa voted and added 1 comment

Votes added by Calder Kitagawa

1 comment

Related details

findit-for-me@appspot.gserviceaccount.com (Gerrit)

findit...@appspot.gserviceaccount.com voted Code-Coverage+1

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information