New issue
Advanced search Search tips

Issue 823797 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 729596



Sign in to add a comment

Prevent posting to uninitialized BrowserThreads

Project Member Reported by gab@chromium.org, Mar 20 2018

Issue description

I realized while fixing BrowserThread::IO to be once again unavailable before CreateThreads (had changed after issue 729596) @ https://chromium-review.googlesource.com/c/chromium/src/+/969104

that nothing bans posting to uninitialized BrowserThreads. It just no-ops.

This is bad because it can result in developers not realizing their task is a no-op or in a unit test not bringing up the environment required to properly run the framework it's driving.

I'm fixing this in https://chromium-review.googlesource.com/c/chromium/src/+/970861
 
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4c59d0aeefab2b990941da360840c06e982caa6

commit d4c59d0aeefab2b990941da360840c06e982caa6
Author: Gabriel Charette <gab@chromium.org>
Date: Wed Mar 21 20:12:26 2018

Introduce TestBrowserThreadBundle::RunUntilIdle()

I'm tired of having to call a static method deep into content/test just
to RunUntilIdle. This brings TestBrowserThreadBundle to feature parity
with ScopedTaskEnvironment in this space which makes sense as the two
are interchangeable.

This also comes closer to robliao's goal of getting rid of
TaskScheduler::FlushForTesting() (used by
content::RunAllTasksUntilIdle()).

And lastly, it makes cleanups in  crbug.com/823797  less ugly as some
of them will need to go from using a ScopedTaskEnvironment to a
TestBrowserThreadBundle and reverting them back to static global flush
is ugly.

R=jam@chromium.org, robliao@chromium.org

Bug:  823797 , 824431
Change-Id: I9f3127f3e46a520fd2d7105ed8139f1046cb3f73
Reviewed-on: https://chromium-review.googlesource.com/971821
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544819}
[modify] https://crrev.com/d4c59d0aeefab2b990941da360840c06e982caa6/content/public/test/test_browser_thread_bundle.cc
[modify] https://crrev.com/d4c59d0aeefab2b990941da360840c06e982caa6/content/public/test/test_browser_thread_bundle.h
[modify] https://crrev.com/d4c59d0aeefab2b990941da360840c06e982caa6/content/public/test/test_browser_thread_bundle_unittest.cc
[modify] https://crrev.com/d4c59d0aeefab2b990941da360840c06e982caa6/content/public/test/test_utils.h

Project Member

Comment 2 by bugdroid1@chromium.org, Mar 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/192b8c9d48bafefd01ab363d28564d14d87b6ab2

commit 192b8c9d48bafefd01ab363d28564d14d87b6ab2
Author: Gabriel Charette <gab@chromium.org>
Date: Wed Mar 21 21:16:00 2018

DCHECK that tasks are only posted to BrowserThreads after they were initialized

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}
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/chrome/browser/chromeos/login/signin_partition_manager_unittest.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/chrome/browser/component_updater/subresource_filter_component_installer_unittest.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/chrome/browser/media/router/discovery/dial/dial_url_fetcher.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/chrome/browser/ssl/ssl_error_handler_unittest.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/content/browser/browser_thread_impl.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/content/browser/loader/url_loader_factory_impl_unittest.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/content/browser/media/capture/desktop_capture_device.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/content/browser/media/capture/desktop_capture_device_unittest.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/content/browser/net/network_quality_observer_impl_unittest.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/content/browser/renderer_host/media/render_frame_audio_input_stream_factory_unittest.cc
[modify] https://crrev.com/192b8c9d48bafefd01ab363d28564d14d87b6ab2/content/browser/renderer_host/render_widget_host_view_mac_editcommand_helper_unittest.mm

Comment 3 by gab@chromium.org, Mar 21 2018

Status: Fixed (was: Started)

Sign in to add a comment