Make in-process gpu init thread safe

Make in-process gpu init thread safe

Initialize in-process gpu before threads are created.
In-process gpu init appends command line switches to the
global current process command line, which is only safe to
do before any background threads start and read the command
line.

BUG=450396
Committed: https://crrev.com/7439d046892d08a0857670a46589b5fa33838e32
Cr-Commit-Position: refs/heads/master@{#313387}

[boliu, 2015-01-26 23:56:09]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2015-01-26 23:56:10]

https://codereview.chromium.org/879703003/diff/1/content/browser/browser_main...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/879703003/diff/1/content/browser/browser_main...
content/browser/browser_main_loop.cc:1029: // On Android,
GLSurface::InitializeOneOff() must be called before initalizing
Pasting your comment over.

On 2015/01/26 19:51:45, sievers wrote:
> Can you move this block into the ifdef-android on the right (1019-1025)?
> And then set |initialize_gpu_data_manager| to false (for Android), so it won't
> get called again below?

Wait, then how will clank initialize GpuDataManager?

> And then you also don't need UsingInProcessGpu().
> 
> And then put a comment for the Android GpuDataManager call site saying why for
> WebView this needs to be initialized so early.

[no sievers, 2015-01-27 00:02:55]

On 2015/01/26 23:56:10, boliu wrote:
>
https://codereview.chromium.org/879703003/diff/1/content/browser/browser_main...
> File content/browser/browser_main_loop.cc (left):
> 
>
https://codereview.chromium.org/879703003/diff/1/content/browser/browser_main...
> content/browser/browser_main_loop.cc:1029: // On Android,
> GLSurface::InitializeOneOff() must be called before initalizing
> Pasting your comment over.
> 
> On 2015/01/26 19:51:45, sievers wrote:
> > Can you move this block into the ifdef-android on the right (1019-1025)?
> > And then set |initialize_gpu_data_manager| to false (for Android), so it
won't
> > get called again below?
> 
> Wait, then how will clank initialize GpuDataManager?


only do it for in-process

> 
> > And then you also don't need UsingInProcessGpu().
> > 
> > And then put a comment for the Android GpuDataManager call site saying why
for
> > WebView this needs to be initialized so early.

[boliu, 2015-01-27 00:15:56]

PS2 doesn't look any better to me. Has more code duplication. Or maybe I didn't
understand what you had in mind?

I think PS1 just needed some comments to be moved around/added?

[no sievers, 2015-01-27 19:43:58]

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:621: // since it's unsafe to manipulate the
global CommandLine after.
nit: I'd say more specifically that we need to append the gpu command line flags
(workarounds etc.) here early when running in-process.

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:627: initialize_gpu_data_manager = false;
Do we really support this or is this fatal?

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:1046: if (!UsingInProcessGpu() &&
!gfx::GLSurface::InitializeOneOff()) {
nit: use |initialize_gpu_data_manager|

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:1049: // crbug.com/326295
Pull this comment out to line 1045 and can you add a TODO(sievers): Shouldn't
need to init full bindings to determine GL version/vendor strings?

[boliu, 2015-01-27 20:06:35]

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:621: // since it's unsafe to manipulate the
global CommandLine after.
On 2015/01/27 19:43:58, sievers wrote:
> nit: I'd say more specifically that we need to append the gpu command line
flags
> (workarounds etc.) here early when running in-process.

Done.

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:627: initialize_gpu_data_manager = false;
On 2015/01/27 19:43:58, sievers wrote:
> Do we really support this or is this fatal?

Webview supports this!

This used be fatal and then someone tried running webview on an emulator with no
gpu support which crashed immediately. So we turned this into a LOG(ERROR).

Now that they are separate, I can make the cross-process one call fatal again.

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:1046: if (!UsingInProcessGpu() &&
!gfx::GLSurface::InitializeOneOff()) {
On 2015/01/27 19:43:58, sievers wrote:
> nit: use |initialize_gpu_data_manager|

Done.

https://codereview.chromium.org/879703003/diff/20001/content/browser/browser_...
content/browser/browser_main_loop.cc:1049: // crbug.com/326295
On 2015/01/27 19:43:58, sievers wrote:
> Pull this comment out to line 1045 and can you add a TODO(sievers): Shouldn't
> need to init full bindings to determine GL version/vendor strings?

Done.

[no sievers, 2015-01-27 20:42:04]

lgtm

https://codereview.chromium.org/879703003/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/879703003/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:626: if
(!gfx::GLSurface::InitializeOneOff()) {
Actually you can comment before this line that this is not thread-safe either
and has to happen before we start threads (for the case where we start a gpu
thread, i.e. in-process, and will call InitializeOneOff() from there also).

https://codereview.chromium.org/879703003/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:634: // otherwise we'll trigger the
assertion about doing IO on the UI thread.
And then maybe this comment can go.

[boliu, 2015-01-27 21:20:15]

https://codereview.chromium.org/879703003/diff/40001/content/browser/browser_...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/879703003/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:626: if
(!gfx::GLSurface::InitializeOneOff()) {
On 2015/01/27 20:42:04, sievers wrote:
> Actually you can comment before this line that this is not thread-safe either
> and has to happen before we start threads (for the case where we start a gpu
> thread, i.e. in-process, and will call InitializeOneOff() from there also).

Done.

https://codereview.chromium.org/879703003/diff/40001/content/browser/browser_...
content/browser/browser_main_loop.cc:634: // otherwise we'll trigger the
assertion about doing IO on the UI thread.
On 2015/01/27 20:42:04, sievers wrote:
> And then maybe this comment can go.

Done.

[boliu, 2015-01-27 21:20:20]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-01-27 21:21:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/879703003/60001

[commit-bot: I haz the power, 2015-01-27 22:51:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-27 22:52:24]

Patchset 4 (id:??) landed as
https://crrev.com/7439d046892d08a0857670a46589b5fa33838e32
Cr-Commit-Position: refs/heads/master@{#313387}