New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 740677 link

Starred by 5 users
Status: Fixed
Owner:
hanxi@chromium.org
Closed: Jun 2018
Cc:
falken@chromium.org
█ roc...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 729596



Sign in to add a comment

Start the IO thread earlier

Project Member Reported by jam@chromium.org, Jul 10 2017 
In order to start services before the rest of the browser (i.e. downloads & networking), we need to start the service manager earlier. That in turn requires starting the IO thread earlier.

Comment 1 by dtapu...@chromium.org, Nov 10 2017

Components: Internals>Core

Comment 2 by yfried...@chromium.org, Nov 22 2017 

I posted some thoughts on crbug/729596
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 12 2018 

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

commit baf7fa879967e836b1b19747de8e8991409ef662
Author: Xi Han <hanxi@google.com>
Date: Thu Apr 12 14:42:18 2018

Setup sandbox host in browser process early.

This is the precursor step before creating a thread to start the
ServiceManager before content_main_runner is running. It is because
no thread is expected to be when the zygote process is forked. In this
CL, we move the setup to ContentMainRunner::Initialize().

Bug:  740677 ,729596
Change-Id: I90e95cce8bc32be03c944aaa17f96c79d555e7cb
Reviewed-on: https://chromium-review.googlesource.com/999741
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550191}
[modify] https://crrev.com/baf7fa879967e836b1b19747de8e8991409ef662/content/app/content_main_runner.cc
[modify] https://crrev.com/baf7fa879967e836b1b19747de8e8991409ef662/content/browser/browser_main_loop.cc
[modify] https://crrev.com/baf7fa879967e836b1b19747de8e8991409ef662/content/browser/sandbox_host_linux.h
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/baf7fa879967e836b1b19747de8e8991409ef662

commit baf7fa879967e836b1b19747de8e8991409ef662
Author: Xi Han <hanxi@google.com>
Date: Thu Apr 12 14:42:18 2018

Setup sandbox host in browser process early.

This is the precursor step before creating a thread to start the
ServiceManager before content_main_runner is running. It is because
no thread is expected to be when the zygote process is forked. In this
CL, we move the setup to ContentMainRunner::Initialize().

Bug:  740677 ,729596
Change-Id: I90e95cce8bc32be03c944aaa17f96c79d555e7cb
Reviewed-on: https://chromium-review.googlesource.com/999741
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550191}
[modify] https://crrev.com/baf7fa879967e836b1b19747de8e8991409ef662/content/app/content_main_runner.cc
[modify] https://crrev.com/baf7fa879967e836b1b19747de8e8991409ef662/content/browser/browser_main_loop.cc
[modify] https://crrev.com/baf7fa879967e836b1b19747de8e8991409ef662/content/browser/sandbox_host_linux.h
Project Member

Comment 5 by bugdroid1@chromium.org, May 10 2018 

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

commit b2c9713cdffc33bc76507a2f01c3e338961e4ff6
Author: Xi Han <hanxi@google.com>
Date: Thu May 10 19:19:17 2018

Rename content_main_runner.cc and browser_main_runner.cc.

This CL is a precursor CL for https://crrev.com/c/969098. It simply contains a
file renaming to keep log history:

1. content_main_runner.cc rename to content_main_runner_impl.cc
2. browser_main_runner.cc rename to browser_main_runner_impl.cc

It allows to introduce content_main_runner_impl.h and browser_main_runner_impl.h
which contain new methods without changing their interface in /content/public.

Bug:  740677 
Change-Id: I4e42bb594d65888b4358ac843b13837be26a5b7e
Reviewed-on: https://chromium-review.googlesource.com/1042456
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557620}
[modify] https://crrev.com/b2c9713cdffc33bc76507a2f01c3e338961e4ff6/chromecast/base/init_command_line_shlib.h
[modify] https://crrev.com/b2c9713cdffc33bc76507a2f01c3e338961e4ff6/content/app/BUILD.gn
[rename] https://crrev.com/b2c9713cdffc33bc76507a2f01c3e338961e4ff6/content/app/content_main_runner_impl.cc
[modify] https://crrev.com/b2c9713cdffc33bc76507a2f01c3e338961e4ff6/content/browser/BUILD.gn
[rename] https://crrev.com/b2c9713cdffc33bc76507a2f01c3e338961e4ff6/content/browser/browser_main_runner_impl.cc
Project Member

Comment 6 by bugdroid1@chromium.org, May 10 2018 

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

commit fc95501fb29e3e7487fff98c17e6aaba1046b330
Author: Xi Han <hanxi@google.com>
Date: Thu May 10 21:24:41 2018

Create BrowserThread::IO thread before browser main loop to start
ServiceManager.

We need a thread to post/execute tasks when starting the
ServiceManager. This thread needs to be created before the browser
main loop is initialized, and will be registered as the
BrowserThread::IO thread which is currently used by ServiceManager
connections.

The creation of such a thread is moved to service_manager::main via
MainDelegate::CreateIOThreadAndGetTaskRunner(). Since it requires no
thread created before calling fork() on posix, we also move the setup
of sandbox before creating the IO thread.

Bug:  740677 , 729596
Change-Id: I23ef57eb52bfb1eb363682dadf98c571c12afcd1
Reviewed-on: https://chromium-review.googlesource.com/969098
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557680}
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/app/BUILD.gn
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/app/content_main_runner_impl.cc
[add] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/app/content_main_runner_impl.h
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/app/content_service_manager_main_delegate.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/app/content_service_manager_main_delegate.h
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/BUILD.gn
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_main.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_main.h
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_main_loop.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_main_loop.h
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_main_loop_unittest.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_main_runner_impl.cc
[add] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_main_runner_impl.h
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_process_sub_thread.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/browser/browser_process_sub_thread.h
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/content/public/test/browser_test_base.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/services/service_manager/embedder/main.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/services/service_manager/embedder/main_delegate.cc
[modify] https://crrev.com/fc95501fb29e3e7487fff98c17e6aaba1046b330/services/service_manager/embedder/main_delegate.h

Comment 7 by hanxi@chromium.org, May 11 2018

Owner: hanxi@chromium.org
Status: Fixed (was: Untriaged)
Project Member

Comment 8 by bugdroid1@chromium.org, May 15 2018 

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

commit 5d76e33d69fa134bf0110781678feac57ae83e5c
Author: Chris Pickel <sfiera@chromium.org>
Date: Tue May 15 12:18:57 2018

Revert "Create BrowserThread::IO thread before browser main loop to start"

This reverts commit fc95501fb29e3e7487fff98c17e6aaba1046b330.

Reason for revert:
Findit (https://goo.gl/kROfz5) identified this CL at revision 557680 as the culprit
for introducing flakiness in the tests as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vZmM5NTUwMWZiMjllM2U3NDg3ZmZmOThjMTdlNmFhYmExMDQ2YjMzMAw

Original change's description:
> Create BrowserThread::IO thread before browser main loop to start
> ServiceManager.
> 
> We need a thread to post/execute tasks when starting the
> ServiceManager. This thread needs to be created before the browser
> main loop is initialized, and will be registered as the
> BrowserThread::IO thread which is currently used by ServiceManager
> connections.
> 
> The creation of such a thread is moved to service_manager::main via
> MainDelegate::CreateIOThreadAndGetTaskRunner(). Since it requires no
> thread created before calling fork() on posix, we also move the setup
> of sandbox before creating the IO thread.
> 
> Bug:  740677 , 729596
> Change-Id: I23ef57eb52bfb1eb363682dadf98c571c12afcd1
> Reviewed-on: https://chromium-review.googlesource.com/969098
> Commit-Queue: Xi Han <hanxi@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557680}

TBR=gab@chromium.org,jam@chromium.org,hanxi@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  740677 , 729596
Change-Id: Ia548067acbf640010f4c8fbed29a0012a274af05
Reviewed-on: https://chromium-review.googlesource.com/1059167
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Commit-Queue: Chris Pickel <sfiera@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558668}
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/app/BUILD.gn
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/app/content_main_runner_impl.cc
[delete] https://crrev.com/a17bbb370bf5597c6d00f2ef6a8bea3ca6e16e38/content/app/content_main_runner_impl.h
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/app/content_service_manager_main_delegate.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/app/content_service_manager_main_delegate.h
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/BUILD.gn
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_main.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_main.h
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_main_loop.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_main_loop.h
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_main_loop_unittest.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_main_runner_impl.cc
[delete] https://crrev.com/a17bbb370bf5597c6d00f2ef6a8bea3ca6e16e38/content/browser/browser_main_runner_impl.h
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_process_sub_thread.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/browser/browser_process_sub_thread.h
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/content/public/test/browser_test_base.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/services/service_manager/embedder/main.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/services/service_manager/embedder/main_delegate.cc
[modify] https://crrev.com/5d76e33d69fa134bf0110781678feac57ae83e5c/services/service_manager/embedder/main_delegate.h

Comment 9 by falken@chromium.org, May 21 2018

Cc: falken@chromium.org
Status: Started (was: Fixed)
I don't have great evidence, but I think https://chromium-review.googlesource.com/969098 may have caused a spike of service worker startup failures.

The failures started in 68.0.3427.0, which contained r557680, and ended in 68.0.3432.0, which contained the revert r558668.

See issue 843456. The startups were failing during a thread hop from the IO to UI thread and back from UI to IO, so it seems plausible.

Please loop me in if the change is relanded, and if you have any theories of why this may have happened.

Comment 10 by hanxi@chromium.org, May 22 2018 

A race condition has existed before my CL, and a fix is in CQ: https://chromium-review.googlesource.com/c/chromium/src/+/1064450. My CL could just enhance it by changing the timings. I will reland the reverted CL, but I will keep an eye on this.
Project Member

Comment 11 by bugdroid1@chromium.org, May 22 2018 

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

commit 6740d6248c24fdb1a591dad3aaaf312919e35fa5
Author: Xi Han <hanxi@google.com>
Date: Tue May 22 19:21:07 2018

Reland "Create BrowserThread::IO thread before browser main loop to start"

This is a reland of I23ef57eb52bfb1eb363682dadf98c571c12afcd1.

It was reverted in Ia548067acbf640010f4c8fbed29a0012a274af05. Reason for revert:
Findit (https://goo.gl/kROfz5) identified this CL at revision 557680 as the culprit
for introducing flakiness in the tests as shown on:
https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vZmM5NTUwMWZiMjllM2U3NDg3ZmZmOThjMTdlNmFhYmExMDQ2YjMzMAw

The data race has been fixed in:
https://chromium-review.googlesource.com/c/chromium/src/+/1064450.

Original change's description:
> Create BrowserThread::IO thread before browser main loop to start
> ServiceManager.
>
> We need a thread to post/execute tasks when starting the
> ServiceManager. This thread needs to be created before the browser
> main loop is initialized, and will be registered as the
> BrowserThread::IO thread which is currently used by ServiceManager
> connections.
>
> The creation of such a thread is moved to service_manager::main via
> MainDelegate::CreateIOThreadAndGetTaskRunner(). Since it requires no
> thread created before calling fork() on posix, we also move the setup
> of sandbox before creating the IO thread.
>
> Bug:  740677 , 729596
> Change-Id: I23ef57eb52bfb1eb363682dadf98c571c12afcd1
> Reviewed-on: https://chromium-review.googlesource.com/969098
> Commit-Queue: Xi Han <hanxi@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557680}


Bug:  740677 , 729596
Change-Id: I9afb0cdc0f11a1d437c2e9bd09c374503c3d5a4b
Reviewed-on: https://chromium-review.googlesource.com/1059949
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560724}
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/app/BUILD.gn
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/app/content_main_runner_impl.cc
[add] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/app/content_main_runner_impl.h
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/app/content_service_manager_main_delegate.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/app/content_service_manager_main_delegate.h
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/BUILD.gn
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_main.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_main.h
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_main_loop.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_main_loop.h
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_main_loop_unittest.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_main_runner_impl.cc
[add] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_main_runner_impl.h
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_process_sub_thread.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/browser/browser_process_sub_thread.h
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/content/public/test/browser_test_base.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/services/service_manager/embedder/main.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/services/service_manager/embedder/main_delegate.cc
[modify] https://crrev.com/6740d6248c24fdb1a591dad3aaaf312919e35fa5/services/service_manager/embedder/main_delegate.h

Comment 12 by hanxi@chromium.org, Jun 5 2018

Status: Fixed (was: Started)
►

Sign in to add a comment