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}
These threads are deprecated and no longer used.
NOPRESUBMIT=true
Bug: 689520
Change-Id: I55b05b70626f91c8a71d56642c851bb786dbfcaa
Reviewed-on: https://chromium-review.googlesource.com/660597
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Francois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501245}
Removes the ScopedTracker calls since we are deleting this infrastructure.
This saves about 25KB in an official build.
Design doc:
https://docs.google.com/document/d/1cibE5nAG9sdjVMfs-MkvrMtp43i9MLGqM_uNusLqiBY
Bug: 762311
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Id00f88d4b7c8c14d51b20d9b53dc233189bdb049
Reviewed-on: https://chromium-review.googlesource.com/651337
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500108}
This CL applies //tools/clang/base_bind_rewriters to //content/browser/{a,b,c,d,f,g}*
It rewrites base::Bind to base::BindOnce where the resulting base::Callback
is immediately converted to base::OnceCallback, which is considered safe
to use base::BindOnce.
E.g.:
base::PostTask(FROM_HERE, base::Bind([]{}));
base::OnceClosure cb = base::Bind([]{});
are converted to:
base::PostTask(FROM_HERE, base::BindOnce([]{}));
base::OnceClosure cb = base::BindOnce([]{});
Change-Id: I11ae9ac169c8e0ff9d9b95452fec22a8b7c91df7
NOPRESUBMIT=true
Change-Id: I11ae9ac169c8e0ff9d9b95452fec22a8b7c91df7
Reviewed-on: https://chromium-review.googlesource.com/628003
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496656}
TaskRunner::RunsTasksOnCurrentThread().
Make TaskRunner::RunsTasksOnCurrentThread() non-virtual and define
it in terms of TaskRunner::RunsTasksInCurrentSequence().
It's the first step to rename RunsTasksOnCurrentThread()
to RunsTasksInCurrentSequence().
BUG=665062
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2823103003
Cr-Commit-Position: refs/heads/master@{#470173}
First CL of a series to split inter-dependency between RunLoop and MessageLoop.
Also modernized bluetooth_socket_bluez_unittest.cc away from using deprecated MessageLoop Quit
methods on its fixture's MessageLoop member to ease transition.
Lastly, tried to add thread safety checks for documentation purposes but turns
out there are already improper usage of the API... those will have to be
addressed first through issue 715235.
https://codereview.chromium.org/2828913003 follows to
s/nested message loop/nested run loop/ in comments.
BUG=703346, 715235
Review-Url: https://codereview.chromium.org/2818533003
Cr-Commit-Position: refs/heads/master@{#469636}
After this CL, TaskRunner::PostTask and its family can take OnceClosure
in addition to Closure.
Most of the changes are mechanical replacement of Closure with OnceClosure
on TaskRunner family. Others are:
- Limit CriticalClosure from Closure to OnceClosure as no caller call
the resulting callback more than once
- Add several PostTaskAndReplyWithResult overloads for old Callback
version, for compatibility. (in base/task_scheduler/post_task.h)
- Update SequencedWorkerPool implementation for OnceClosure.
- Update task handling code in app_state.mm for OnceClosure, which is
needed to bring OnceClosure into a ObjC block.
BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2637843002
Cr-Commit-Position: refs/heads/master@{#462023}
This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.
With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.
Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.
task_runner->PostTask(FROM_HERE, base::Bind(&Foo));
base::Closure cb = base::Bind(&Foo);
task_runner->PostTask(FROM_HERE, std::move(cb));
Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.
base::Closure cb = base::Bind(&Foo);
task_runner->PostTask(FROM_HERE, cb);
This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.
TBR=shrike@chromium.org
BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2726523002
Cr-Commit-Position: refs/heads/master@{#460288}
This is a less controversial part of OnceCallback migration of PostTaskAndReply.
https://codereview.chromium.org/2657603004/
After this CL PostTaskAndReply family take the callback by value, and
move it to their storage. That reduces the chance to leave a reference
to the callback on the original thread.
Review-Url: https://codereview.chromium.org/2678303002
Cr-Commit-Position: refs/heads/master@{#448965}
This is part of the migration phase for TaskScheduler, design doc:
https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa5k/edit
Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop
and id in favor of TaskRunners and BrowserThreadState.
Cool side-effect: BrowserThreadImpl::StartWithOptions() no longer blocks on the
underlying thread starting (it used to via GetThreadId()), it now takes a reference
to its TaskRunner and lets the thread itself start on its own schedule :).
This wasn't previously intended to block (BrowserThreadImpl::StartAndWaitForTesting() is
supposed to be used for that) but had unintentionally regressed in http://crrev.com/408295
because GetThreadId() implicitly waits (http://crbug.com/672977).
Another nice improvement is that ~BrowserThreadImpl() now cleans the BrowserThreadGlobals
associated with it, this has the side-effect to force proper destruction order of
TestBrowserThread in tests which brought forth a few precursor cleanups
(https://crbug.com/653916#c24) which will from now on be enforced by this CL.
When redirection is disabled, the logic should be the exact same as before.
- Threads are brought up.
- Tasks are posted to their MessageLoop (albeit through their TaskRunner now).
- On shutdown, threads are joined one by one.
When redirection is enabled, we try to mimic this logic.
- Redirection TaskRunners are only live (|accepting_tasks|) for the same period that the
matching thread would be.
- On shutdown, we block on each TaskRunner's queue being flushed one-by-one
in the same order as in the no-redirection case (this almost identical to
real threads join % one slight difference documented in detail in
BrowserThreadImpl::StopRedirectionOfThreadID()).
BUG=653916, 672977
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
TEST=
A) "out\Release\chrome.exe"
Runs/shuts down the exact same as before.
B) "out\Release\chrome.exe --force-fieldtrials=BrowserScheduler/Enabled/ --force-fieldtrial-params=BrowserScheduler.Enabled:Background/3;3;1;0;10000/BackgroundFileIO/3;3;1;0;10000/Foreground/3;3;1;0;10000/ForegroundFileIO/3;3;1;0;10000/RedirectSequencedWorkerPools/true/RedirectNonUINonIOBrowserThreads/true"
Runs/shuts down smoothly and chrome://tracing confirms that redirected BrowserThreads are running on TaskSchedulerWorkers.
Review-Url: https://codereview.chromium.org/2464233002
Cr-Commit-Position: refs/heads/master@{#439139}
The IO thread was already the only user and officially restricting users
of this API is required as BrowserThreadDelegates won't be compatible
with threads redirected to the TaskScheduler.
BUG=653916
Review-Url: https://codereview.chromium.org/2558943002
Cr-Commit-Position: refs/heads/master@{#437271}
Replacing one weird MessageLoop::IsIdleForTesting() loop in
chrome/browser/ui/app_list/speech_recognizer_browsertest.cc
with content::RunAllPendingInMessageLoop(content::BrowserThread::IO);
And all other use cases by BrowserThread::GetTaskRunnerForThread().
BUG=661344
Review-Url: https://codereview.chromium.org/2473823002
Cr-Commit-Position: refs/heads/master@{#429774}
This CL disallows task observers on the DB, FILE, FILE_USER_BLOCKING,
PROCESS_LAUNCHER and CACHE threads. Calling Add/RemoveTaskObserver
on these threads will result in a crash.
This is important because these threads will be migrated to
TaskScheduler which doesn't support task observers.
To our knowledge, no code uses task observers on these threads. This
CL will help us confirm that in the wild.
BUG=653916
Review-Url: https://codereview.chromium.org/2419803002
Cr-Commit-Position: refs/heads/master@{#425418}
This CL disallows nesting on BrowserThreads that will be migrated
to TaskScheduler. Running a nested loop or calling
Add/RemoveNestingObserver on these threads will result in a crash.
To our knowledge, no code runs a nested loop on BrowserThreads that
will be migrated to TaskScheduler. This CL will help us confirm that
in the wild.
BUG=653916
Review-Url: https://codereview.chromium.org/2407313002
Cr-Commit-Position: refs/heads/master@{#424749}
