Addresses ~14% of `-WUnsafe-buffer-usage` opt-out in `//content`.
This is a #cleanup.
The patch was initially partially generated by `./tool/clang/spanify`.
This patch applies conversions specifically to c-arrays in `//content`.
Although the `spanify` tool supports broader conversions, this change
intentionally focuses on c-arrays. Several manual fixes were made to
address anonymous struct issues and other edge cases encountered during
the conversion process.
Bug: 342213636, 40285824
Change-Id: I72666faa45a461ea27bb3608beeacc6c9fcd8a63
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5824992
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1349804}
This is similar to the GUARDED_BY_CONTEXT() macro.
It can be used to ensure at compile time that every access to
a variable is checked with a DCHECK_CURRENTLY_ON() or
CHECK_CURRENTLY_ON() call.
Change-Id: Ic69ee85550613043b71c616e4770b1d178e3f8d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5738775
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1335038}
Suppress unsafe buffer usage on a file-by-file basis. Out of
approximately 5850 .cc and .h files only roughly 160 files fail
compilation with the unsafe buffers warning.
Suppress only, by inserting boilerplate into affected files. Do not
re-write any code to work around the issues. Properly fixing each file
will be done in follow-up CLs.
//content/ is not removed from unsafe_bufers_paths.txt file and will be
also done as a follow-up, so it makes potential reverts simpler.
Bug: 342213636
Change-Id: I4a936e63dea95a78951f7bfae6d5487708ae3c0b
AX-Relnotes: n/a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5608913
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1312393}
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}
This is in preparation for landing crrev.com/c/5213211 where a
CHECK_CURRENTLY_ON macro is being introduced and this function name is
awkward and orthogonal to whether it's CHECKed or DCHECKed.
Bug: 1459899
Change-Id: I60a2a4ede6b083caea12d6e273be9e35db176e20
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5282255
Commit-Queue: Peter Boström <pbos@chromium.org>
Auto-Submit: Peter Boström <pbos@chromium.org>
Reviewed-by: Ali Juma <ajuma@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Ali Juma <ajuma@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1258765}
Now that the circular dependency between browser_thread.h and
browser_task_traits.h has been resolved, browser_thread.h can include
browser_task_traits.h and provide a default ({}) for BrowserTaskTraits.
A bunch of includes in other files needed to be updated.
Bug: 1424788, 1026641, 1429044
Change-Id: Ibc44e495f743eee029fb45832b1084f03ca47555
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4402191
Reviewed-by: Gabriel Charette <gab@chromium.org>
Owners-Override: Gabriel Charette <gab@chromium.org>
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1127199}
Turns out there's a lot of includes, so these will have to be removed
before deleting the implementation of the task runner handles.
To allow the deletion of the task runner handle headers, add
the sequenced/thread task runner handles where they are used in
the codebase with scripts.
This was done with an automated change, with a few touchups afterwards.
The code for the mass-refactor changes are here:
python:
https://paste.googleplex.com/5534570878337024
shell:
https://paste.googleplex.com/6466750748033024
In terms of touchups:
- add sequenced/thread task runner handles to
the third_party/blink/public/DEPS, because multiple files were using
it transitively anyways.
- rewrite certain parts of the codebase which used
ThreadTaskRunnerHandles instead of CurrentDefaultHandles.
- fix a compile issue with forward-declaration in
extensions/browser/extension_file_task_runner.h.
AX-Relnotes: n/a.
Bug: 1026641
Change-Id: I737ef32aee4e77c21eaa3a2bdc403a28322cf1b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4133323
Owners-Override: Gabriel Charette <gab@chromium.org>
Commit-Queue: Sean Maher <spvw@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1090532}
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}
Note to QA: This CL is purely mechanical and shouldn't be blamed
for future regressions on touched files.
This is a follow-up to https://chromium-review.googlesource.com/c/chromium/src/+/2211138
which already removed all usage using content::BrowserThread.
Hence this script now matches unqualified BrowserThread:: without
risking having "content::" be selected as "traits_before" by the regex
(ran on same revision as step #1).
content:: is now always added if outside namespace content {}
(deleting unused using content::BrowserThread; decls)
Script @ https://crbug.com/1026641#c92
(will TBR fdoray@ post-review for mechanical change)
TBR=fdoray@hchromium.org
AX-Relnotes: n/a.
Bug: 1026641
Change-Id: I51ae2f83eb17d19b54563fd9b4fc040d2aa0c948
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2212469
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772458}
Note to QA: This CL is purely mechanical and shouldn't be blamed
for future regressions on touched files.
Only migrate explicit usage of content::BrowserThread in step #1 as
these are the only ones that clearly map to a fully qualified call to
content::Get(UI|IO)ThreadTaskRunner().
Script @ https://crbug.com/1026641#c91
Will make BrowserTaskTraits optional when empty in a follow-up
(can't before end of migration because of cyclic dependency between
browser_thread.h and browser_task_traits.h).
(will TBR fdoray@ post-review for mechanical change)
TBR=fdoray@chromium.org
AX-Relnotes: n/a.
Bug: 1026641
Change-Id: I27615773472c97d9bfd981c8c9e783c26525b51a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2211138
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772055}
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}
We'd like to turn off sequence manager anti-starvation logic but we
need to change this first to ensure the expected ordering of
PostTask and content::BrowserThread::DeleteSoon.
The observable difference is BrowserThread::GetCurrentThreadIdentifier
now inherits base::TaskPriority where previously it did not which is
dangerous.
Bug: 1013535, 1019767
Change-Id: I88cf03635b37f680f10b5aefd582afae35474a45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1886901
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710883}
This is
1) s/TestBrowserThreadBundle/BrowserTaskEnvironment/ on src/
2) s/([^/])\b\w*thread_bundle(_)?/\1task_environment\2/ on files modified in (1)
3) git cl format
4) Manually fix up remainder "ThreadBundle" and "thread_bundle" found via
string search on the whole of src/.
For ease of review:
* Step #3 is the diff between patch sets 1..2
* Step #4 is the diff between patch sets 2..3
TBR=dcheng@chromium.org
(for post-review owners bypass per mechanical change)
Bug: 992483
Change-Id: I4945141f6d78bdc6c98444198e5012ddc8e5bff0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1758440
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688755}
*** Note: There is no behavior change from this patch. ***
The PostTask APIs will shortly be changed to require all tasks to explicitly
specify their thread affinity, i.e., whether the task should run on the thread
pool or a specific named thread such as a BrowserThread. This patch updates all
call sites with thread affinity annotation. We also remove the "WithTraits"
suffix to make the call sites more readable.
Before:
// Thread pool task.
base::PostTaskWithTraits(FROM_HERE, {...}, ...);
// UI thread task.
base::PostTaskWithTraits(FROM_HERE, {BrowserThread::UI, ...}, ...);
After:
// Thread pool task.
base::PostTask(FROM_HERE, {base::ThreadPool(), ...}, ...);
// UI thread task.
base::PostTask(FROM_HERE, {BrowserThread::UI, ...}, ...);
This patch was semi-automatically prepared with these steps:
1. Patch in https://chromium-review.googlesource.com/c/chromium/src/+/1635827
to make thread affinity a build-time requirement.
2. Run an initial pass with a clang rewriter:
https://chromium-review.googlesource.com/c/chromium/src/+/1635623
3. ninja -C out/Debug | grep 'requested here' | cut -d: -f1-3 | sort | \
uniq > errors.txt
4. while read line; do
f=$(echo $line | cut -d: -f 1)
r=$(echo $line | cut -d: -f 2)
c=$(echo $line | cut -d: -f 3)
sed -i "${r}s/./&base::ThreadPool(),/$c" $f
done < errors.txt
5. GOTO 3 until build succeeds.
6. Remove the "WithTraits" suffix from task API call sites:
$ tools/git/mffr.py -i <(cat <<EOF
[
["PostTaskWithTraits", "PostTask"],
["PostDelayedTaskWithTraits", "PostDelayedTask"],
["PostTaskWithTraitsAndReply", "PostTaskAndReply"],
["CreateTaskRunnerWithTraits", "CreateTaskRunner"],
["CreateSequencedTaskRunnerWithTraits", "CreateSequencedTaskRunner"],
["CreateUpdateableSequencedTaskRunnerWithTraits", "CreateUpdateableSequencedTaskRunner"],
["CreateSingleThreadTaskRunnerWithTraits", "CreateSingleThreadTaskRunner"],
["CreateCOMSTATaskRunnerWithTraits", "CreateCOMSTATaskRunner"]
]
EOF
)
This CL was uploaded by git cl split.
R=boliu@chromium.org, tsepez@chromium.org
Bug: 968047
Change-Id: I346372d16a3856186ea74d14e0dd8a12f7cacae5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1729589
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Auto-Submit: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683554}
This helper will be useful when migrating code away from
PostAfterStartupTask. Some callsites would otherwise need a trampoline
as they need to post to a given task runner. This helper is only needed
until we have sequence-funneling in place.
This patch is a split of https://crrev.com/c/1571661. Refer to this if
you need more context for this change.
Bug: 887407
Change-Id: I122f837e6a7b509903055798deb0f2b6eb215bed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1628725
Commit-Queue: Carlos Caballero <carlscab@google.com>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663717}
Soon there will be multiple queues with different priorities, so the
trick of posting a task that quits a run loop currentlyl in use in
test_utils.cc will no longer work. In preparation for that this patch
add a better implementation of the same functionality.
Bug: 863341
Change-Id: I643e22a491145a3bae00d06ae9164a450f501298
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1541251
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Carlos Caballero <carlscab@google.com>
Cr-Commit-Position: refs/heads/master@{#646252}
This is in preparation for the BrowserUIThreadScheduler where we
hope to have only one location where it's set up.
Bug: 863341, 872372
Change-Id: Ia7c07adc1d60280f2aed25f5226bfce80c6aba4b
Reviewed-on: https://chromium-review.googlesource.com/c/1461393
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#632797}
This moves the proxy TaskRunners into BrowserTaskExecutor, which
then exposes them to base/post_task.h.
Also replaces uses of LazyInstance with NoDestructor.
Bug: 878356
Change-Id: I4ca14d49404158da9f98953b3a725da47081881d
Reviewed-on: https://chromium-review.googlesource.com/1236437
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593868}
This removes BrowserThread::Post(NonNestable|)(Delayed|)Task and moves
GetTaskRunnerForThread into the impl (it's still used to vend
TaskRunners in BrowserTaskExecutor).
These methods have been replaced by base/post_task.h in conjunction
with content/public/browser/browser_task_traits.h
Bug: 878356
Change-Id: Ia122dc0921769f43da9271ddd6e1ce2f402df779
Reviewed-on: https://chromium-review.googlesource.com/1235728
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592879}
The two methods converged over the last year and are now equivalent.
Bug: 825054
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4ec5f7afb5b7b21278af6c777efe9ad57d2aae68
Reviewed-on: https://chromium-review.googlesource.com/977063
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546529}
Sampling profiler highlights high contention on this lock:
crbug.com/821034.
Initializing/tear down was already single-threaded (tear down had to be moved
from BrowserProcessSubThread::Cleanup() to ~BrowserProcessSubThread() to
enforce this with a ThreadChecker however). Add dchecks to confirm/enforce
this and remove the lock which is usuless on data structures that are now (as
of series of changes over the last year) read-only after startup.
Use atomics to protect reads across the RUNNING=>SHUTDOWN boundary.
Bug: 821034
Change-Id: I7800048bff51ad79cb10ee89fd3a0a31534c393e
Reviewed-on: https://chromium-review.googlesource.com/973556
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546212}
This is a reland of d260e9cf66
It was reverted because of crbug.com/824716, these weren't new crashes
but known crashes mislabeled as a fallout of this change.
http://cl/190471699 fixes the crash backend to not rely on
"content::BrowserThreadImpl::IOThreadRun" being in the signature.
Only diff in this CL is to use base::debug::Alias() in methods that we
don't want optimized (i.e. IOThreadRun) out as CHECK_GT was seen as
optimized out in some of the reported crashes (even though the same
pattern as before was used by this CL..?)
Original change's description:
> Refactor BrowserThreadImpl, BrowserProcessSubThread, and BrowserMainLoop
>
> This brings back the invariant that BrowserThread::IO isn't available
> before BrowserMainLoop::CreateThreads(). This was broken to fix issue
> 729596 to bring up the thread earlier for ServiceManager but it is
> important that code that posts to BrowserThread::IO statically have an
> happens-after relationship to BrowserMainLoop::CreateThreads(). Exposing
> it statically earlier put that invariant at risk.
>
> Thankfully fixing issue 815225 resulted in finally reaching the long
> sought goal of only having BrowserThread::UI/IO. Now that the IO thread
> is also kicked off before it's named statically, BrowserThreadImpl no
> longer needs to be a base::Thread, hence this refactoring.
>
> Before this CL:
> * BrowserThreadImpl was a base::Thread
> (could be a fake thread if SetMessageLoop was used)
> * BrowserProcessSubThread was a BrowserThreadImpl
> (performed a bit more initialization)
> * BrowserProcessSubThread was only used in production (in
> BrowserMainLoop)
> * BrowserThreadImpl was used for fake threads (BrowserMainLoop for
> BrowserThread::UI) and for testing (TestBrowserThread(Impl)).
> * BrowserThreadImpl overrode Init/Run/CleanUp() from base::Thread to
> perform some sanity checks as well as drive IOThread's Delegate (ref.
> BrowserThread::SetIOThreadDelegate())
> * BrowserProcessSubThread re-overrode Init/Run/CleanUp() to perform
> per-thread //content initialization (tests missed out on that per
> TestBrowserThread bypassing BrowserProcessSubThread by directly
> subclassing BrowserThreadImpl).
>
> With this CL:
> * BrowserThreadImpl is merely a scoped object that binds a provided
> SingleThreadTaskRunner to a BrowserThread::ID.
> * BrowserProcessSubThread is a base::Thread and performs all of the
> initialization and cleanup specific to //content (this means it now
> also manages BrowserThread::SetIOThreadDelegate())
> * BrowserProcessSubThread can be brought up early before being bound to
> a BrowserThread::ID (BrowserMainLoop handles that through
> BrowserProcessSubThread ::RegisterAsBrowserThread())
>
> Unfortunate exceptions required for this CL:
> * IOThread::Init() (invoked through BrowserThreadDelegate) perfoms
> blocking operations this was previously performed before installed
> the ThreadRestrictions on BrowserThread::IO. But now that //content
> is initialized after bringing up the thread, a
> base::ScopedAllowBlocking is required in scope of IOThread::Init().
> * TestBrowserThread previously bypassing BrowserProcessSubThread by
> directly subclassing BrowserThreadImpl meant it wasn't subject to
> ThreadRestrictions (unfortunate becomes it denies allowance
> verification to product code running in unit tests). Adding it back
> causes DCHECKs, as such
> BrowserProcessSubThread::AllowBlockingForTesting was added to allow
> this CL to pass CQ.
>
> Of note:
> * BrowserProcessSubThread is still written as though it supports many
> BrowserThread::IDs but in practice it's mostly always
> BrowserThread::IO (except in ThreadWatcherTest I think). This change
> was big enough that I didn't bother also breaking that
> generalization.
> * BrowserThreadImpl's constructor was made private to ensure only
> BrowserProcessSubThread and a few select callers get to drive it (to
> avoid previous missed initialization issues)
> * Atomics to manage BrowserThread::SetIOThreadDelegate were removed.
> Restriction was instead added that this only be called before
> initialization and after shutdown (this was already the case).
>
> Follow-ups to this CL:
> * //ios duplicates this logic and will need to undergo the same change
> as a follow-up
> * Fixing ios will allow removal of base::Thread::SetMessageLoop hack :)
> * Removing BrowserThreadGlobals::lock_ to address crbug.com/821034 will
> be much easier
> * BrowserThread post APIs should DCHECK rather than no-op if using a
> BrowserThread::ID before it's registered.
>
> Bug: 815225, 821034, 729596
> Change-Id: If1038f23079df72203b1e95c7d26647f8824a726
> Reviewed-on: https://chromium-review.googlesource.com/969104
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544440}
TBR=jam@chromium.org
Bug: 815225, 821034, 729596, 824716
Change-Id: I9a180975c69a008f8519d1d3d44663aa58a74a92
Reviewed-on: https://chromium-review.googlesource.com/980793
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546104}
This reverts commit d260e9cf66.
Reason for revert: In Windows Canary versions since 67.0.3377.0, where this commit landed, IO thread hang reports have spiked dramatically. It is now the #1 browser crash report on Windows Canary at 33% of reports. I'm speculatively reverting this to see if the crash rate heals.
Original change's description:
> Refactor BrowserThreadImpl, BrowserProcessSubThread, and BrowserMainLoop
>
> This brings back the invariant that BrowserThread::IO isn't available
> before BrowserMainLoop::CreateThreads(). This was broken to fix issue
> 729596 to bring up the thread earlier for ServiceManager but it is
> important that code that posts to BrowserThread::IO statically have an
> happens-after relationship to BrowserMainLoop::CreateThreads(). Exposing
> it statically earlier put that invariant at risk.
>
> Thankfully fixing issue 815225 resulted in finally reaching the long
> sought goal of only having BrowserThread::UI/IO. Now that the IO thread
> is also kicked off before it's named statically, BrowserThreadImpl no
> longer needs to be a base::Thread, hence this refactoring.
>
> Before this CL:
> * BrowserThreadImpl was a base::Thread
> (could be a fake thread if SetMessageLoop was used)
> * BrowserProcessSubThread was a BrowserThreadImpl
> (performed a bit more initialization)
> * BrowserProcessSubThread was only used in production (in
> BrowserMainLoop)
> * BrowserThreadImpl was used for fake threads (BrowserMainLoop for
> BrowserThread::UI) and for testing (TestBrowserThread(Impl)).
> * BrowserThreadImpl overrode Init/Run/CleanUp() from base::Thread to
> perform some sanity checks as well as drive IOThread's Delegate (ref.
> BrowserThread::SetIOThreadDelegate())
> * BrowserProcessSubThread re-overrode Init/Run/CleanUp() to perform
> per-thread //content initialization (tests missed out on that per
> TestBrowserThread bypassing BrowserProcessSubThread by directly
> subclassing BrowserThreadImpl).
>
> With this CL:
> * BrowserThreadImpl is merely a scoped object that binds a provided
> SingleThreadTaskRunner to a BrowserThread::ID.
> * BrowserProcessSubThread is a base::Thread and performs all of the
> initialization and cleanup specific to //content (this means it now
> also manages BrowserThread::SetIOThreadDelegate())
> * BrowserProcessSubThread can be brought up early before being bound to
> a BrowserThread::ID (BrowserMainLoop handles that through
> BrowserProcessSubThread ::RegisterAsBrowserThread())
>
> Unfortunate exceptions required for this CL:
> * IOThread::Init() (invoked through BrowserThreadDelegate) perfoms
> blocking operations this was previously performed before installed
> the ThreadRestrictions on BrowserThread::IO. But now that //content
> is initialized after bringing up the thread, a
> base::ScopedAllowBlocking is required in scope of IOThread::Init().
> * TestBrowserThread previously bypassing BrowserProcessSubThread by
> directly subclassing BrowserThreadImpl meant it wasn't subject to
> ThreadRestrictions (unfortunate becomes it denies allowance
> verification to product code running in unit tests). Adding it back
> causes DCHECKs, as such
> BrowserProcessSubThread::AllowBlockingForTesting was added to allow
> this CL to pass CQ.
>
> Of note:
> * BrowserProcessSubThread is still written as though it supports many
> BrowserThread::IDs but in practice it's mostly always
> BrowserThread::IO (except in ThreadWatcherTest I think). This change
> was big enough that I didn't bother also breaking that
> generalization.
> * BrowserThreadImpl's constructor was made private to ensure only
> BrowserProcessSubThread and a few select callers get to drive it (to
> avoid previous missed initialization issues)
> * Atomics to manage BrowserThread::SetIOThreadDelegate were removed.
> Restriction was instead added that this only be called before
> initialization and after shutdown (this was already the case).
>
> Follow-ups to this CL:
> * //ios duplicates this logic and will need to undergo the same change
> as a follow-up
> * Fixing ios will allow removal of base::Thread::SetMessageLoop hack :)
> * Removing BrowserThreadGlobals::lock_ to address crbug.com/821034 will
> be much easier
> * BrowserThread post APIs should DCHECK rather than no-op if using a
> BrowserThread::ID before it's registered.
>
> Bug: 815225, 821034, 729596
> Change-Id: If1038f23079df72203b1e95c7d26647f8824a726
> Reviewed-on: https://chromium-review.googlesource.com/969104
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544440}
TBR=gab@chromium.org,jam@chromium.org
NOPRESUBMIT=true
# Not skipping CQ checks because original CL landed > 1 day ago.
# falken: Skipping presubmit to use deprecated ThreadResrictions::DisallowWaiting().
Bug: 815225, 821034, 729596
Change-Id: I2be97c5d8183497c005ab397c871f625b034d850
Reviewed-on: https://chromium-review.googlesource.com/979752
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545725}
Posting is still allowed in the SHUTDOWN phase in production (will no-op
when after the thread is torn down as usual) but it is again disallowed
after BrowserThreadImpl::ResetGlobalsForTesting (i.e. after
~TestBrowserThreadBundle).
content/browser/browser_thread_impl.cc :
- Core change to deny PostTask to uninitialized BrowserThreads
(shutdown is fine). In practice this prevents posting before
BrowserMainLoop::CreateThreads in production and outside the scope of
a TestBrowserThreadBundle in tests (and after
~TestBrowserThreadBundle() which fully uninitializes after shutdown).
chrome/browser/chrome_content_browser_client.cc :
- ChromeContentBrowserClient::SetApplicationLocale can happen
synchronously if IO thread is not yet initialized (change check to
reflect proper thread).
content/browser/loader/url_loader_factory_impl_unittest.cc :
- Make sure TestBrowserThreadBundle outlives members as well.
content/browser/media/capture/desktop_capture_device.cc
chrome/browser/media/router/discovery/dial/dial_url_fetcher.cc
content/browser/media/capture/desktop_capture_device_unittest.cc :
- crbug.com/823869
content/browser/net/network_quality_observer_impl_unittest.cc :
- NetworkQualityObserverImpl's constructor uses BrowserThreads, would
previously no-op and not break the test but this change brings it
closer to testing the same code as in production
content/browser/renderer_host/media/render_frame_audio_input_stream_factory_unittest.cc
chrome/browser/ssl/ssl_error_handler_unittest.cc
chrome/browser/chromeos/login/signin_partition_manager_unittest.cc
content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm :
- See inline comments
chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc :
- Trivial upgrade from ScopedTaskEnvironment as some logic does use
BrowserThreads.
Bug: 823797, 823869
Change-Id: Iab99220606714362959a00820ecd46334eae7c8a
Reviewed-on: https://chromium-review.googlesource.com/970861
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544845}
This brings back the invariant that BrowserThread::IO isn't available
before BrowserMainLoop::CreateThreads(). This was broken to fix issue
729596 to bring up the thread earlier for ServiceManager but it is
important that code that posts to BrowserThread::IO statically have an
happens-after relationship to BrowserMainLoop::CreateThreads(). Exposing
it statically earlier put that invariant at risk.
Thankfully fixing issue 815225 resulted in finally reaching the long
sought goal of only having BrowserThread::UI/IO. Now that the IO thread
is also kicked off before it's named statically, BrowserThreadImpl no
longer needs to be a base::Thread, hence this refactoring.
Before this CL:
* BrowserThreadImpl was a base::Thread
(could be a fake thread if SetMessageLoop was used)
* BrowserProcessSubThread was a BrowserThreadImpl
(performed a bit more initialization)
* BrowserProcessSubThread was only used in production (in
BrowserMainLoop)
* BrowserThreadImpl was used for fake threads (BrowserMainLoop for
BrowserThread::UI) and for testing (TestBrowserThread(Impl)).
* BrowserThreadImpl overrode Init/Run/CleanUp() from base::Thread to
perform some sanity checks as well as drive IOThread's Delegate (ref.
BrowserThread::SetIOThreadDelegate())
* BrowserProcessSubThread re-overrode Init/Run/CleanUp() to perform
per-thread //content initialization (tests missed out on that per
TestBrowserThread bypassing BrowserProcessSubThread by directly
subclassing BrowserThreadImpl).
With this CL:
* BrowserThreadImpl is merely a scoped object that binds a provided
SingleThreadTaskRunner to a BrowserThread::ID.
* BrowserProcessSubThread is a base::Thread and performs all of the
initialization and cleanup specific to //content (this means it now
also manages BrowserThread::SetIOThreadDelegate())
* BrowserProcessSubThread can be brought up early before being bound to
a BrowserThread::ID (BrowserMainLoop handles that through
BrowserProcessSubThread ::RegisterAsBrowserThread())
Unfortunate exceptions required for this CL:
* IOThread::Init() (invoked through BrowserThreadDelegate) perfoms
blocking operations this was previously performed before installed
the ThreadRestrictions on BrowserThread::IO. But now that //content
is initialized after bringing up the thread, a
base::ScopedAllowBlocking is required in scope of IOThread::Init().
* TestBrowserThread previously bypassing BrowserProcessSubThread by
directly subclassing BrowserThreadImpl meant it wasn't subject to
ThreadRestrictions (unfortunate becomes it denies allowance
verification to product code running in unit tests). Adding it back
causes DCHECKs, as such
BrowserProcessSubThread::AllowBlockingForTesting was added to allow
this CL to pass CQ.
Of note:
* BrowserProcessSubThread is still written as though it supports many
BrowserThread::IDs but in practice it's mostly always
BrowserThread::IO (except in ThreadWatcherTest I think). This change
was big enough that I didn't bother also breaking that
generalization.
* BrowserThreadImpl's constructor was made private to ensure only
BrowserProcessSubThread and a few select callers get to drive it (to
avoid previous missed initialization issues)
* Atomics to manage BrowserThread::SetIOThreadDelegate were removed.
Restriction was instead added that this only be called before
initialization and after shutdown (this was already the case).
Follow-ups to this CL:
* //ios duplicates this logic and will need to undergo the same change
as a follow-up
* Fixing ios will allow removal of base::Thread::SetMessageLoop hack :)
* Removing BrowserThreadGlobals::lock_ to address crbug.com/821034 will
be much easier
* BrowserThread post APIs should DCHECK rather than no-op if using a
BrowserThread::ID before it's registered.
Bug: 815225, 821034, 729596
Change-Id: If1038f23079df72203b1e95c7d26647f8824a726
Reviewed-on: https://chromium-review.googlesource.com/969104
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544440}
ServiceManager may need to launch child processes before a BrowserMainLoop
has been created, and therefore before any BrowserThreads exists. We
therefore replace the BrowserThread::PROCESS_LAUNCHER with a global
TaskScheduler sequence, created on-demand when the ServiceManager or
ChildProcessLauncherHelper first need it.
Under Windows we must use a single-thread TaskRunner, while on other
platforms we are able to use a normal TaskScheduler sequence. File
crbug.com/820200 to track that.
Bug: 815225
Change-Id: Ia0f46461fb9cc92fddacf81ee96b764de8477d11
Reviewed-on: https://chromium-review.googlesource.com/941264
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542212}
In this CL, we split the creation of the BrowserThread::IO thread and
initialization of its delegate:
1. The BrowserThread::IO is created in the
BrowserMainLoop::PostMainMessageLoopStart() after the UI thread
is created. Adjust the tracing events in the BrowserMainLoop.
2. Simplify the logic in BrowserMainLoop::CreateThreads() since only
the launcher thread is created there. Initializes the
delegate of the BrowserThread::IO there.
3. Update the logic in the BrowserThreadImpl when checking whether
the BrowserThread::IO has initialized.
4. Update BrowserMainLoopTest and BrowserProcessImplTest.
This is the first step of starting ServiceManager earlier.
Bug: 729596
Change-Id: I7a08fd7c7bf5548f8039c8c38152ecfde3bd20c4
Reviewed-on: https://chromium-review.googlesource.com/905424
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537318}
disk_cache users should only need to go through disk_cache.h;
the individual _impl.h files aren't public API.
Change-Id: Ie2e7f696ec81e03ff00dbfcfc5535fd7929acf83
Reviewed-on: https://chromium-review.googlesource.com/916742
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536713}
The LazyInstance::operator== removed by this CL is undesirable, because
1) it didn't have a corresponding operator!=
2) it was only used to compare against nullptr
(i.e. to check if the lazy instance was created already or not)
This CL removes LazyInstance::operator== and replaces it with
LazyInstance::IsCreated() method.
Bug: 789738
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Idfbb4a9f0699cb10f3f861860070b7f316a16ad6
Reviewed-on: https://chromium-review.googlesource.com/792124
Reviewed-by: Nick Carter <nick@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522172}
This reverts commit 6127a86f3a.
Reason for revert: Suspect this caused spike in ThreadWatcher hang reports: crbug.com/768886
Original change's description:
> Remove all remaining deprecated Web/BrowserThreads!!
>
> (except BrowserThread::FILE which has a handful use cases left still and
> will follow suit shortly)
>
> (BrowserThread::PROCESS_LAUNCHER migration was put on hold because of
> Android specific requirements around process launching)
>
> Remaining usage of TestBrowserThreadBundle::REAL_DB_THREAD could simply
> be removed as all of the product code was already migrated to TaskScheduler
> which always uses real threads in unit tests.
>
> Bug: 689520, 365909, 752144
> Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
> Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be
>
> NOPRESUBMIT=True (for content/browser/browser_thread_impl.cc:211)
>
> Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be
> Reviewed-on: https://chromium-review.googlesource.com/610880
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503732}
TBR=gab@chromium.org,jam@chromium.org,dpranke@chromium.org,asvitkine@chromium.org,sdefresne@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
NOPRESUBMIT=True (for restoring the deprecated browser threads)
Bug: 689520, 365909, 752144, 768886
Change-Id: I53d93572118303743e1dde71b2a473350717b215
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Reviewed-on: https://chromium-review.googlesource.com/689254
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504939}
(except BrowserThread::FILE which has a handful use cases left still and
will follow suit shortly)
(BrowserThread::PROCESS_LAUNCHER migration was put on hold because of
Android specific requirements around process launching)
Remaining usage of TestBrowserThreadBundle::REAL_DB_THREAD could simply
be removed as all of the product code was already migrated to TaskScheduler
which always uses real threads in unit tests.
Bug: 689520, 365909, 752144
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be
NOPRESUBMIT=True (for content/browser/browser_thread_impl.cc:211)
Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be
Reviewed-on: https://chromium-review.googlesource.com/610880
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503732}
