Prevent posting to uninitialized BrowserThreads |
||
Issue descriptionI 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
,
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
,
Mar 21 2018
|
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Mar 21 2018