Do not create threads from PreCreateThreads |
|||||
Issue descriptionChromeBrowserMainParts::PreCreateThreadsImpl does this. At the end of ChromeBrowserMainParts::PreCreateThreadsImpl we initialize the TaskScheduler (and thus some of its threads). This is wrong because content:: rightfully assumes no threads were created yet and continues with its own PreCreateThreads that has logic based on that valid assumption, e.g.: // 1) Need to initialize in-process GpuDataManager before creating threads. // It's unsafe to append the gpu command line switches to the global // CommandLine::ForCurrentProcess object after threads are created. // 2) Must be after parts_->PreCreateThreads to pick up chrome://flags. GpuDataManagerImpl* gpu_data_manager = GpuDataManagerImpl::GetInstance(); gpu_data_manager->Initialize(); https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?rcl=0&l=782 Options: 1) Expose BrowserMainParts::CreateThreads() which would allow parts to initialize their threads after PreCreateThreads but before content's threads -- perhaps overkill given the single use case. 2) Change the BrowserMainParts::PreCreateThreads() API's documentation to guarantee that parts_->PreCreateThreads() is the last thing invoked before CreateThreads() -- i.e. content::BrowserMainLoop::PreCreateThreads() runs its stuff before calling parts_'s and parts_->PreCreateThreads() can finish by creating threads for itself before content creates its own. 3) Move TaskScheduler initialization to content::, blockers: A) It requires components/variations/ but it looks to me like content/browser/ could depend on this per comment in content/DEPS. B) The order at the end of ChromeBrowserMainParts::PreCreateThreadsImpl() is currently crucial (browser_process_->PreCreateThreads() must happen after SetupMetrics() which must happen after MaybeInitializeTaskScheduler()), this could potentially be tweaked. I'm leaning towards (2). @jam: WDYT?
,
Nov 3 2016
We will eventually have to move TaskScheduler initialization to content:: if we want cast https://cs.chromium.org/chromium/src/chromecast/browser/cast_browser_main_parts.h, headless https://cs.chromium.org/chromium/src/headless/lib/browser/headless_browser_main_parts.h and Android Webview https://cs.chromium.org/chromium/src/android_webview/browser/aw_browser_main_parts.h to use it. If content/ can't depend on components/variations/, it could always get variations params through a new method on ChromeContentBrowserClient. Refactoring SetupMetrics() so it doesn't have to be after TaskScheduler initialization (and thus can stay in PreCreateThreadsImpl() even if TaskScheduler initialization is moved to BrowserMainLoop::CreateThreads()) is certainly feasible.
,
Nov 7 2016
,
Nov 15 2016
,
Nov 15 2016
,
Dec 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512 commit 7253fd2e30a62a81e3638f0bdbf1c5dacfd03512 Author: robliao <robliao@chromium.org> Date: Thu Dec 01 18:41:38 2016 Decouple Metrics Initialization from IO Thread Initialization In preparation of moving Task Scheduler initialization to the content CreateThreads phase, metrics has to be initialized during PreMainMessageLoopRun. The only reference to metrics during PreCreateThreads was in the IO Thread state initialization to set up a IO thread to UI thread forwarder that reported network connection state (e.g. cellular or not). This change moves that initialization to be called on demand as the callback is not needed until the first network URL request is satisfied. This happens after CreateThreads and generally after PreMainMessageLoopRun. BUG= 662052 Review-Url: https://codereview.chromium.org/2517363002 Cr-Commit-Position: refs/heads/master@{#435657} [modify] https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512/chrome/browser/io_thread.cc [modify] https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512/chrome/browser/io_thread.h [modify] https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512/components/metrics/data_use_tracker.cc [modify] https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512/components/metrics/data_use_tracker.h [modify] https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512/components/metrics/data_use_tracker_unittest.cc [modify] https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512/components/metrics/metrics_service.cc [modify] https://crrev.com/7253fd2e30a62a81e3638f0bdbf1c5dacfd03512/components/metrics/metrics_service.h
,
Dec 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1892c914719015f06c135127fd1dfd13dec39d33 commit 1892c914719015f06c135127fd1dfd13dec39d33 Author: robliao <robliao@chromium.org> Date: Thu Dec 01 18:59:51 2016 Move SetupMetrics() to PreMainMessageLoopRun This allows the Browser Task Scheduler to initialize during CreateThreads since SetupMetrics() ultimately requires use of the blocking pool. The blocking pool has to be redirected by the Browser Task Scheduler before this point BUG= 662052 Review-Url: https://codereview.chromium.org/2514373003 Cr-Commit-Position: refs/heads/master@{#435664} [modify] https://crrev.com/1892c914719015f06c135127fd1dfd13dec39d33/chrome/browser/chrome_browser_main.cc
,
Dec 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/521cf9b934c0d82f5935ce7e9536b0b31e5ef381 commit 521cf9b934c0d82f5935ce7e9536b0b31e5ef381 Author: robliao <robliao@chromium.org> Date: Thu Dec 08 01:26:15 2016 Split initialization_util into a Hermetic Library and a Variations Library This split allows callers to easily use some of the task_scheduler_init structures without taking a dependency on variations. Also adds unit tests for some of the functions. BUG= 662052 TBR=asvitkine Review-Url: https://codereview.chromium.org/2553953002 Cr-Commit-Position: refs/heads/master@{#437113} [modify] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/BUILD.gn [modify] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/BUILD.gn [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/initialization/BUILD.gn [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/initialization/DEPS [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/initialization/browser_util.cc [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/initialization/browser_util.h [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/initialization/browser_util_unittest.cc [modify] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/initialization_util.cc [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/variations/BUILD.gn [rename] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/variations/DEPS [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/variations/browser_variations_util.cc [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/variations/browser_variations_util.h [add] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/components/task_scheduler_util/variations/browser_variations_util_unittest.cc [modify] https://crrev.com/521cf9b934c0d82f5935ce7e9536b0b31e5ef381/testing/variations/fieldtrial_testing_config.json
,
Dec 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf5a32eecdb817a66b45af678f11ca1368d648b1 commit bf5a32eecdb817a66b45af678f11ca1368d648b1 Author: robliao <robliao@chromium.org> Date: Fri Dec 09 03:35:46 2016 Move Task Scheduler Initialization From chrome/browser to Content Content appears to actually be the owner of the process running a browser. As such, it stands to reason that it should be the one initializing and controlling the lifetime of the task scheduler. BUG= 662052 Review-Url: https://codereview.chromium.org/2539263003 Cr-Commit-Position: refs/heads/master@{#437452} [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/chrome/browser/BUILD.gn [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/chrome/browser/chrome_content_browser_client.cc [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/chrome/browser/chrome_content_browser_client.h [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/components/task_scheduler_util/initialization/browser_util.cc [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/components/task_scheduler_util/initialization/browser_util.h [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/components/task_scheduler_util/initialization_util.cc [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/components/task_scheduler_util/initialization_util.h [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/content/browser/browser_main_loop.cc [modify] https://crrev.com/bf5a32eecdb817a66b45af678f11ca1368d648b1/content/public/browser/content_browser_client.h
,
Dec 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e39f2306910b799fae4537e9e0e97f44121e1333 commit e39f2306910b799fae4537e9e0e97f44121e1333 Author: robliao <robliao@chromium.org> Date: Wed Dec 14 00:51:49 2016 Move SetupMetrics() to PreMainMessageLoopRun This allows the Browser Task Scheduler to initialize during CreateThreads since SetupMetrics() ultimately requires use of the blocking pool. The blocking pool has to be redirected by the Browser Task Scheduler before this point Originating Chrome Change: http://crrev.com/1892c914719015f06c135127fd1dfd13dec39d33 BUG= 662052 Review-Url: https://codereview.chromium.org/2569863003 Cr-Commit-Position: refs/heads/master@{#438367} [modify] https://crrev.com/e39f2306910b799fae4537e9e0e97f44121e1333/ios/chrome/browser/ios_chrome_main_parts.mm
,
Dec 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/792b01c0f3d5c1bc6f1ced7d1385da7684367140 commit 792b01c0f3d5c1bc6f1ced7d1385da7684367140 Author: robliao <robliao@chromium.org> Date: Wed Dec 14 01:00:44 2016 Move Task Scheduler Initialization From ios/chrome/browser to Web Web appears to actually be the owner of the process running a browser. As such, it stands to reason that it should be the one initializing and controlling the lifetime of the task scheduler. This mirrors the Chrome change at https://codereview.chromium.org/2539263003/ BUG= 662052 Review-Url: https://codereview.chromium.org/2570693003 Cr-Commit-Position: refs/heads/master@{#438370} [delete] https://crrev.com/e234d53ddf514554e22e02186141c81a019e7832/components/task_scheduler_util/BUILD.gn [delete] https://crrev.com/e234d53ddf514554e22e02186141c81a019e7832/components/task_scheduler_util/initialization_util.cc [delete] https://crrev.com/e234d53ddf514554e22e02186141c81a019e7832/components/task_scheduler_util/initialization_util.h [modify] https://crrev.com/792b01c0f3d5c1bc6f1ced7d1385da7684367140/ios/chrome/browser/BUILD.gn [modify] https://crrev.com/792b01c0f3d5c1bc6f1ced7d1385da7684367140/ios/chrome/browser/DEPS [modify] https://crrev.com/792b01c0f3d5c1bc6f1ced7d1385da7684367140/ios/chrome/browser/ios_chrome_main_parts.mm [modify] https://crrev.com/792b01c0f3d5c1bc6f1ced7d1385da7684367140/ios/web/app/web_main_loop.mm [modify] https://crrev.com/792b01c0f3d5c1bc6f1ced7d1385da7684367140/ios/web/public/web_client.h
,
Dec 14 2016
The following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/5f66c1975c14fb5f7e4c2748b2d85aef8cda5fa3 commit 5f66c1975c14fb5f7e4c2748b2d85aef8cda5fa3 Author: robliao <robliao@google.com> Date: Wed Dec 14 19:30:58 2016
,
Dec 14 2016
The following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/5f66c1975c14fb5f7e4c2748b2d85aef8cda5fa3 commit 5f66c1975c14fb5f7e4c2748b2d85aef8cda5fa3 Author: robliao <robliao@google.com> Date: Wed Dec 14 19:30:58 2016
,
Dec 14 2016
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by gab@chromium.org
, Nov 3 2016