NOTREACHED() and NOTREACHED_IN_MIGRATION() are both CHECK-fatal now.
The former is [[noreturn]] so this CL also performs dead-code removal
after the NOTREACHED().
This CL does not attempt to do additional rewrites of any surrounding
code, like:
if (!foo) {
NOTREACHED();
}
to CHECK(foo);
Those transforms take a non-trivial amount of time (and there are
thousands of instances). Cleanup can be left as an exercise for the
reader.
This does clean up kCrashOnDanglingBrowserContext as both paths of the
kill switch are currently fatal. This has been rolled out for a long
time.
Bug: 40580068, 40062641
Change-Id: Ib88e710d003e2e48df3fc502ca54d2341d157a0e
Cq-Include-Trybots: luci.chromium.try:linux-dcheck-off-rel
Low-Coverage-Reason: OTHER Should-be-unreachable code
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5974816
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1376522}
This was generated by replacing " NOTREACHED()" with
" NOTREACHED_IN_MIGRATION()" and running git cl format.
This prepares for making NOTREACHED() [[noreturn]] alongside
NotReachedIsFatal migration of existing inventory.
Bug: 40580068
Change-Id: I3b48b89911ac5e9ffcb211622992f917f8f9e8d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5539619
Auto-Submit: Peter Boström <pbos@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Owners-Override: Lei Zhang <thestig@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1301096}
The canonical bug format is TODO(crbug.com/<id>). TODOs of the
following forms will all be migrated to the new format:
- TODO(crbug.com/<old id>)
- TODO(https://crbug.com/<old id>)
- TODO(crbug/<old id>)
- TODO(crbug/monorail/<old id>)
- TODO(<old id>)
- TODO(issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/<old id>)
- TODO(https://issues.chromium.org/u/1/issues/<old id>)
- TODO(bugs.chromium.org/<old id>)
Bug id mapping is sourced from go/chrome-on-buganizer-prod-issues.
See go/crbug-todo-migration for details.
#crbug-todo-migration
Bug: b/321899722
Change-Id: Ibc66b8c440e4bcdef414e77fef4d9874d2ea9951
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5493800
Auto-Submit: Alison Gale <agale@chromium.org>
Commit-Queue: Alison Gale <agale@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Owners-Override: Alison Gale <agale@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1293330}
This reverts commit 269aa9d493.
Reason for revert: including fix for test
Original change's description:
> Revert "Add code supporting new field-trial-internals page"
>
> This reverts commit b8e462bc4b.
>
> Reason for revert: This CL is likely the cause of test failure CfmBrowserServiceTest.GetVariationsData on https://ci.chromium.org/ui/p/chromium/builders/ci/linux-cfm-rel starting from https://ci.chromium.org/ui/b/8763776059209925809. Error message: Expected equality of these values:
> field_trial_states
> Which is: "*Baz/Qux/Foo/Bar/"
> states
> Which is: "*Baz/Qux/Foo/Bar"
>
> Original change's description:
> > Add code supporting new field-trial-internals page
> >
> > This CL lays the groundwork for a future
> > field-trial-internals page, with the following changes:
> >
> > * Update base::FieldTrial to include an 'overridden' flag,
> > which is used to change variation group hashes so
> > overridden trial hashes are distinct.
> > * I pulled out code to build field trial strings
> > into AppendFieldTrialGroupToString, to be reused. In some places, we
> > were adding a trailing '/', and other places we weren't. Now we omit
> > the trailing slash everywhere. We can still parse the string
> > with or without the trailing slash.
> >
> > See go/field-trial-internals-dd for more information.
> >
> > Bug: b:284986126
> > Change-Id: I58b86c558cd7973bb6a493322353b52ebf9f619f
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4620752
> > Reviewed-by: Richard (Torne) Coles <torne@chromium.org>
> > Reviewed-by: Matt Menke <mmenke@chromium.org>
> > Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
> > Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
> > Commit-Queue: Dan H <harringtond@chromium.org>
> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> > Reviewed-by: Erik Chen <erikchen@chromium.org>
> > Reviewed-by: Luc Nguyen <lucnguyen@google.com>
> > Cr-Commit-Position: refs/heads/main@{#1227624}
>
> Bug: b:284986126
> Change-Id: I18f84ccf307f6265da24b3603b82344e1bf42c20
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5052200
> Owners-Override: Maggie Cai <mxcai@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Commit-Queue: Maggie Cai <mxcai@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1227726}
Bug: b:284986126
Change-Id: Id00f514233c08aa439caaa7f4d6bdaa6c5fc38d5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5055166
Reviewed-by: Luc Nguyen <lucnguyen@google.com>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Paul Moy <pmoy@chromium.org>
Reviewed-by: Richard (Torne) Coles <torne@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1229416}
This reverts commit b8e462bc4b.
Reason for revert: This CL is likely the cause of test failure CfmBrowserServiceTest.GetVariationsData on https://ci.chromium.org/ui/p/chromium/builders/ci/linux-cfm-rel starting from https://ci.chromium.org/ui/b/8763776059209925809. Error message: Expected equality of these values:
field_trial_states
Which is: "*Baz/Qux/Foo/Bar/"
states
Which is: "*Baz/Qux/Foo/Bar"
Original change's description:
> Add code supporting new field-trial-internals page
>
> This CL lays the groundwork for a future
> field-trial-internals page, with the following changes:
>
> * Update base::FieldTrial to include an 'overridden' flag,
> which is used to change variation group hashes so
> overridden trial hashes are distinct.
> * I pulled out code to build field trial strings
> into AppendFieldTrialGroupToString, to be reused. In some places, we
> were adding a trailing '/', and other places we weren't. Now we omit
> the trailing slash everywhere. We can still parse the string
> with or without the trailing slash.
>
> See go/field-trial-internals-dd for more information.
>
> Bug: b:284986126
> Change-Id: I58b86c558cd7973bb6a493322353b52ebf9f619f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4620752
> Reviewed-by: Richard (Torne) Coles <torne@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
> Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
> Commit-Queue: Dan H <harringtond@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Luc Nguyen <lucnguyen@google.com>
> Cr-Commit-Position: refs/heads/main@{#1227624}
Bug: b:284986126
Change-Id: I18f84ccf307f6265da24b3603b82344e1bf42c20
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5052200
Owners-Override: Maggie Cai <mxcai@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Maggie Cai <mxcai@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1227726}
This CL lays the groundwork for a future
field-trial-internals page, with the following changes:
* Update base::FieldTrial to include an 'overridden' flag,
which is used to change variation group hashes so
overridden trial hashes are distinct.
* I pulled out code to build field trial strings
into AppendFieldTrialGroupToString, to be reused. In some places, we
were adding a trailing '/', and other places we weren't. Now we omit
the trailing slash everywhere. We can still parse the string
with or without the trailing slash.
See go/field-trial-internals-dd for more information.
Bug: b:284986126
Change-Id: I58b86c558cd7973bb6a493322353b52ebf9f619f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4620752
Reviewed-by: Richard (Torne) Coles <torne@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Luc Nguyen <lucnguyen@google.com>
Cr-Commit-Position: refs/heads/main@{#1227624}
This adds an is_low_anonymity property of FieldTrials, and by default
does not return these to callers (in GetActiveFieldTrialGroups() and
via Observers). Callers which specifically want to read the low
anonymity trials must do so via FieldTrialListIncludingLowAnonymity.
This also includes a change to move away from the deprecated
`base::LazyInstance`, required to avoid a build failure as a
side-effect of this CL (see crbug.com/990948).
Bug: b/274900786
Change-Id: Ic5f275856bc57b2e4b660f219cd9c43b99e832cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4386467
Commit-Queue: James Lee <ljjlee@google.com>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1130880}
This helper was created in
commit 7b509e56dc to expedite the UI/IO
thread simplification effort, and particularly the service worker thread
migration which is now complete. It was not intended as a long-term
function for general use, as it is a strange layer over the normal
threading and tasks functions in base/ and encourages a typically
unrecommended pattern of "maybe async, maybe sync". There are just a
few targeted uses of it today, and these can directly use the usual
functions without this helper.
Bug: 1138155, 1187753
Change-Id: I7c82f0909656a802ec103b2af40bceb436e26e57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3173372
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/main@{#924711}
== Changes since original CL ==
This CL registers ChildProcessFieldTrialSyncer as a FieldTrialList
observer instead of ChildThreadImpl and it makes
ChildProcessFieldTrialSyncer a leaky singleton. This eliminates
lifetime issues.
An alternative would have been to remove ChildThreadImpl from the list
of FieldTrialList observers upon destruction. We decided not to do that
because it would have required extra synchronization.
Diff: https://crrev.com/c/2867567/1..16
== Original description ==
This CL removes usage of ObserverListThreadSafe::NotifySynchronously
in FieldTrialList, to support the removal of this method in a
separate CL.
Currently, FieldTrialList uses
ObserverListThreadSafe::NotifySynchronously to notify observers when a
group is selected for a field trial. This method dispatches synchronous
notifications to observers which were registered on the current
sequence, and uses PostTask to notify other observers asynchronously.
Through code inspection, we found that all implementations of
FieldTrialList::Observer::OnFieldTrialGroupFinalized are
thread-safe. Therefore, they can always be notified synchronously,
no matter which sequence they were registered from.
This CL makes these changes:
- Observers are stored in a lock-protected std::vector instead of
in an ObserverListThreadSafe.
- To notify observers, the list of observers is copied to a local
variable while holding the lock. Then, after releasing the
lock, all observers in the local list are notified.
- In the scope where observers are notified, an atomic variable is
incremented. The RemoveObserver method checks that the variable is
equal to zero. This ensures that no observer is removed while
dispatching notifications (which could cause a use-after-free).
For reference, the implementations of
FieldTrialList::Observer::OnFieldTrialGroupFinalized are:
base/android/field_trial_list.cc:
LOG(INFO) is thread-safe
base/metrics/field_trial_unittest.cc:
The test is single-threaded.
components/variations/child_process_field_trial_syncer_unittest.cc:
The test is single-threaded.
components/variations/variations_crash_keys.cc:
The observer checks whether it runs on the UI thread and
forwards the notification to the UI thread if it's not the case.
components/variations/variations_ids_provider.cc:
The observer uses a lock.
content/browser/field_trial_synchronizer.cc:
The observer checks whether it runs on the UI thread and
forwards the notification to the UI thread if it's not the case.
content/browser/net/network_field_trial_browsertest.cc
RunLoop::Quit() is thread-safe.
content/child/child_thread_impl.cc
A mojo::SharedRemote is thread-safe.
Bug: 1193750
Change-Id: I8351a2cbbce4d5deaafbe2aa75f902e6da822d53
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2867567
Commit-Queue: François Doray <fdoray@chromium.org>
Auto-Submit: François Doray <fdoray@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880105}
This CL removes usage of ObserverListThreadSafe::NotifySynchronously
in FieldTrialList, to support the removal of this method in a
separate CL.
Currently, FieldTrialList uses
ObserverListThreadSafe::NotifySynchronously to notify observers when a
group is selected for a field trial. This method dispatches synchronous
notifications to observers which were registered on the current
sequence, and uses PostTask to notify other observers asynchronously.
Through code inspection, we found that all implementations of
FieldTrialList::Observer::OnFieldTrialGroupFinalized are
thread-safe. Therefore, they can always be notified synchronously,
no matter which sequence they were registered from.
This CL makes these changes:
- Observers are stored in a lock-protected std::vector instead of
in an ObserverListThreadSafe.
- To notify observers, the list of observers is copied to a local
variable while holding the lock. Then, after releasing the
lock, all observers in the local list are notified.
- In the scope where observers are notified, an atomic variable is
incremented. The RemoveObserver method checks that the variable is
equal to zero. This ensures that no observer is removed while
dispatching notifications (which could cause a use-after-free).
For reference, the implementations of
FieldTrialList::Observer::OnFieldTrialGroupFinalized are:
base/android/field_trial_list.cc:
LOG(INFO) is thread-safe
base/metrics/field_trial_unittest.cc:
The test is single-threaded.
components/variations/child_process_field_trial_syncer_unittest.cc:
The test is single-threaded.
components/variations/variations_crash_keys.cc:
The observer checks whether it runs on the UI thread and
forwards the notification to the UI thread if it's not the case.
components/variations/variations_ids_provider.cc:
The observer uses a lock.
content/browser/field_trial_synchronizer.cc:
The observer checks whether it runs on the UI thread and
forwards the notification to the UI thread if it's not the case.
content/browser/net/network_field_trial_browsertest.cc
RunLoop::Quit() is thread-safe.
content/child/child_thread_impl.cc
A mojo::SharedRemote is thread-safe.
Bug: 1193750
Change-Id: I0144d3262fca360fbd2a48580e322428510f91d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2785140
Auto-Submit: François Doray <fdoray@chromium.org>
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#870762}
This CL is a no-op.
Expose BrowserThread::GetTaskRunnerForThread helper and use it to
migrate remaining callers of the old BrowserThread APIs going through
base::.
All other callers using static BrowserTaskTraits (i.e. pre-determined
at compile time, no variables) have already been migrated, these
are the only ones remaining.
Bug: 1026641
Change-Id: I7d068872bda21810e430ce4844f728e4e94c1b35
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2787574
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867554}
This implements the new GOOGLE_WEB_PROPERTIES_FIRST_PARTY
and GOOGLE_WEB_PROPERTIES_TRIGGER_FIRST_PARTY variations
types, which allow us to limit the set of experiment IDs included in
X-Client-Data headers sent in third-party contexts.
Adding support for this involved sending a map of headers rather than a
single header to the renderer. A map is used because the request context
is unknown until the process of sending the request has started. So, the
renderer needs to know the appropriate headers to send in first- and
third-party contexts.
Bug: 1094303
Change-Id: I447219b1cc80b0dc8552b440e6ad07b2fdb15d8b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411489
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Auto-Submit: Caitlin Fischer <caitlinfischer@google.com>
Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
Cr-Commit-Position: refs/heads/master@{#809756}
The status quo implementation generates two types,
cached_variation_ids_header_signed_in_ and cached_variation_ids_header_,
which are stored in strings.
In order to limit the number of VariationIDs included in the header in
third-party contexts, four headers are needed. The headers are stored in
a map.
FYI: While GetClientDataHeader()'s |web_visibility| argument is only
Study_GoogleWebVisibility_ANY for the time being, this will not always
be the case.
This change paves the way for limiting the set of experiment IDs
included in X-Client-Data headers sent in third-party contexts.
Bug: 1094303
Change-Id: Ifdc502fbc25fc3b09911414198022e059132a83c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2388763
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Caitlin Fischer <caitlinfischer@google.com>
Cr-Commit-Position: refs/heads/master@{#807875}
VariationsClient::IsIncognito returns true for all off-the-record
profiles and is always initialized with BrowserContext::IsOffTheRecord.
To remove misinterpretations, the function is renamed to IsOffTheRecord.
This CL does not create any behavioral change.
Bug: 968028, 1074201
Change-Id: I19a0a81794e7838c8f882faa6aeab9d7b085f756
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2202980
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Ramin Halavati <rhalavati@chromium.org>
Cr-Commit-Position: refs/heads/master@{#769614}
CHECK, CHECK_EQ etc., and NOTREACHED/NOTIMPLEMENTED have moved
to the much smaller headers check.h, check_op.h, and notreached.h,
respectively.
This CL updates .cc files to use those headers instead when
possible, with the purpose of saving compile time.
(Split out from https://crrev.com/c/2164525 which also has
notes on how the change was generated.)
Bug: 1031540
Change-Id: I643818242b92e19a1048fac89dd8aae323e8b1ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2164510
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Hans Wennborg <hans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763511}
This allows it to be shared by Chrome, WebLayer and WebView. For Chrome
this functionality is tested by VariationsHttpHeadersBrowserTest.
To support this a VariationsClient has been added, with an accessor on
BrowserContext, which allows the variations code to tell if the user is
signed in (some of the variations header logic needs to know this).
Bug: 1025612
Change-Id: Ibb5a0dd54e23a3536de4c0fb84624173a6974262
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1967245
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745404}
