[Android] Initialize CommandLine before any usage in Render.

[Android] Initialize CommandLine before any usage in Render.

On low-end devices, if Chrome was upgraded from a previous version,
Chrome decides to use normal mode to keep user experience consistent.
This is done by force '--low-end-device-mode=0' on both browser and render
processes.

However, when crazy linker is used, Linker.isUsed() triggers
SysUtils.detectLowEndDevice() call which checks CommandLine first.
Existing code invokes Linker.isUsed() before initilizing CommandLine,
which causes the render process ignores '--low-end-device-mode=0'.

This results the browser in normal mode, but render is in low-end device mode.

BUG=424228
Committed: https://crrev.com/0dfe9ba256030d15cc897295c3fa3bdd5d564404
Cr-Commit-Position: refs/heads/master@{#301581}

[Feng Qian, 2014-10-21 19:41:42]

feng@chromium.org changed reviewers:
+ simonb@chromium.org, tedchoc@chromium.org

[Ted C, 2014-10-21 20:00:02]

lgtm -- seems reasonable to me, but definitely wait for simonb@ before
submitting.

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(left):

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:258:
if (Linker.isUsed())
Removing this check isn't strictly needed though right?

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:149:
if (mLinkerParams != null) {
With your removal of the check below, I don't think this should ever be possible
anymore.

[Feng Qian, 2014-10-21 20:19:59]

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(left):

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:258:
if (Linker.isUsed())
On 2014/10/21 20:00:02, Ted C wrote:
> Removing this check isn't strictly needed though right?

Yeah, mLinkerParams is only used when Linker.isUsed() is true.

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/665273003/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:149:
if (mLinkerParams != null) {
On 2014/10/21 20:00:02, Ted C wrote:
> With your removal of the check below, I don't think this should ever be
possible
> anymore.

Acknowledged.

[simonb (inactive), 2014-10-22 13:08:10]

lgtm

Thanks for fixing.

https://codereview.chromium.org/665273003/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/665273003/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:258:
mLinkerParams = new ChromiumLinkerParams(intent);
Maybe add a comment that mLinkerParams is never used if it turns out that (after
parsing the command line?) Linker.isUsed() is false.

[Feng Qian, 2014-10-22 21:25:27]

feng@chromium.org changed reviewers:
+ nyquist@chromium.org

[Feng Qian, 2014-10-22 21:25:27]

Ted & Simon,

I made a small change in SysUtils.detectLowEndDevice that turns if
(CommnadLine.isInitialized()) to "assert CommandLine.isInitialized()" so that we
won't hit the case later.

+nyquist, who is one of owners of base/android directory.

[nyquist, 2014-10-23 22:30:48]

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SysUtils.java:106: assert
CommandLine.isInitialized();
How does this work for the ChildProcessLauncher?

The static member field
org.chromium.content.browser.ChildProcessLauncher#sBindingManager is statically
initialized though this call hierarchy:
org.chromium.content.browser.BindingManagerImpl#createBindingManager
org.chromium.base.SysUtils#isLowEndDevice
org.chromium.base.SysUtils#detectLowEndDevice

Are we sure that static initializer is never run before the CommandLine has been
initialized?

[Feng Qian, 2014-10-23 22:57:26]

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SysUtils.java:106: assert
CommandLine.isInitialized();
ChildProcessLauncher is called in browser process and CommandLine was
initialized already.

The assert here is to catch and prevent the case isLowEndDevice from being
called but command line wasn't initialized.

The try bot doesn't break tests, so I assume existing code wasn't broken by this
CL, and future violation should be caught by new assertion.


On 2014/10/23 22:30:48, nyquist wrote:
> How does this work for the ChildProcessLauncher?
> 
> The static member field
> org.chromium.content.browser.ChildProcessLauncher#sBindingManager is
statically
> initialized though this call hierarchy:
> org.chromium.content.browser.BindingManagerImpl#createBindingManager
> org.chromium.base.SysUtils#isLowEndDevice
> org.chromium.base.SysUtils#detectLowEndDevice
> 
> Are we sure that static initializer is never run before the CommandLine has
been
> initialized?

[nyquist, 2014-10-24 18:14:42]

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SysUtils.java:106: assert
CommandLine.isInitialized();
The class is initialized whenever: 
- An instance of ChildProcessLauncher is created.
- A static method declared by ChildProcessLauncher is invoked.
- A static field declared by ChildProcessLauncher is assigned.
- A static field declared by ChildProcessLauncher is used and the field is not a
constant variable.
- An assert statement lexically nested within ChildProcessLauncher is executed.
(from http://docs.oracle.com/javase/specs/jls/se7/html/jls-12.html#jls-12.4)

And then because of the static member in ChildProcessLauncher:
private static BindingManager sBindingManager =
BindingManagerImpl.createBindingManager();
and createBindingManager() calls into SysUtils#isLowEndDevice() ->
SysUtils#detectLowEndDevice(), any code accessing static methods on
ChildProcessLauncher (say warmUp, onSentToBackground, setInForeground,
onBrougtToForeground) would cause a crash in debug builds.

Is that your intention? If it is, then I think that is fine with me. Just wanted
to check.

On 2014/10/23 22:57:26, Feng Qian wrote:
> ChildProcessLauncher is called in browser process and CommandLine was
> initialized already.
> 
> The assert here is to catch and prevent the case isLowEndDevice from being
> called but command line wasn't initialized.
> 
> The try bot doesn't break tests, so I assume existing code wasn't broken by
this
> CL, and future violation should be caught by new assertion.
> 
> 
> On 2014/10/23 22:30:48, nyquist wrote:
> > How does this work for the ChildProcessLauncher?
> > 
> > The static member field
> > org.chromium.content.browser.ChildProcessLauncher#sBindingManager is
> statically
> > initialized though this call hierarchy:
> > org.chromium.content.browser.BindingManagerImpl#createBindingManager
> > org.chromium.base.SysUtils#isLowEndDevice
> > org.chromium.base.SysUtils#detectLowEndDevice
> > 
> > Are we sure that static initializer is never run before the CommandLine has
> been
> > initialized?
>

[Feng Qian, 2014-10-27 16:07:30]

On 2014/10/24 18:14:42, nyquist wrote:
>
https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
> File base/android/java/src/org/chromium/base/SysUtils.java (right):
> 
>
https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
> base/android/java/src/org/chromium/base/SysUtils.java:106: assert
> CommandLine.isInitialized();
> The class is initialized whenever: 
> - An instance of ChildProcessLauncher is created.
> - A static method declared by ChildProcessLauncher is invoked.
> - A static field declared by ChildProcessLauncher is assigned.
> - A static field declared by ChildProcessLauncher is used and the field is not
a
> constant variable.
> - An assert statement lexically nested within ChildProcessLauncher is
executed.
> (from http://docs.oracle.com/javase/specs/jls/se7/html/jls-12.html#jls-12.4)
> 
> And then because of the static member in ChildProcessLauncher:
> private static BindingManager sBindingManager =
> BindingManagerImpl.createBindingManager();
> and createBindingManager() calls into SysUtils#isLowEndDevice() ->
> SysUtils#detectLowEndDevice(), any code accessing static methods on
> ChildProcessLauncher (say warmUp, onSentToBackground, setInForeground,
> onBrougtToForeground) would cause a crash in debug builds.
> 
> Is that your intention? If it is, then I think that is fine with me. Just
wanted
> to check.

Yes, that's intended, we should prevent any path that leads to
detectLowEndDevice
without initializing command line. Otherwise, we will get inconsistent logic
of dealing with device class.


> 
> On 2014/10/23 22:57:26, Feng Qian wrote:
> > ChildProcessLauncher is called in browser process and CommandLine was
> > initialized already.
> > 
> > The assert here is to catch and prevent the case isLowEndDevice from being
> > called but command line wasn't initialized.
> > 
> > The try bot doesn't break tests, so I assume existing code wasn't broken by
> this
> > CL, and future violation should be caught by new assertion.
> > 
> > 
> > On 2014/10/23 22:30:48, nyquist wrote:
> > > How does this work for the ChildProcessLauncher?
> > > 
> > > The static member field
> > > org.chromium.content.browser.ChildProcessLauncher#sBindingManager is
> > statically
> > > initialized though this call hierarchy:
> > > org.chromium.content.browser.BindingManagerImpl#createBindingManager
> > > org.chromium.base.SysUtils#isLowEndDevice
> > > org.chromium.base.SysUtils#detectLowEndDevice
> > > 
> > > Are we sure that static initializer is never run before the CommandLine
has
> > been
> > > initialized?
> >

[Feng Qian, 2014-10-27 16:09:17]

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/SysUtils.java (right):

https://codereview.chromium.org/665273003/diff/60001/base/android/java/src/or...
base/android/java/src/org/chromium/base/SysUtils.java:106: assert
CommandLine.isInitialized();
Yes, that's intended, we want to catch all cases where detectLowEndDevice is
called but command line wasn't initialized.


On 2014/10/24 18:14:42, nyquist wrote:
> The class is initialized whenever: 
> - An instance of ChildProcessLauncher is created.
> - A static method declared by ChildProcessLauncher is invoked.
> - A static field declared by ChildProcessLauncher is assigned.
> - A static field declared by ChildProcessLauncher is used and the field is not
a
> constant variable.
> - An assert statement lexically nested within ChildProcessLauncher is
executed.
> (from http://docs.oracle.com/javase/specs/jls/se7/html/jls-12.html#jls-12.4)
> 
> And then because of the static member in ChildProcessLauncher:
> private static BindingManager sBindingManager =
> BindingManagerImpl.createBindingManager();
> and createBindingManager() calls into SysUtils#isLowEndDevice() ->
> SysUtils#detectLowEndDevice(), any code accessing static methods on
> ChildProcessLauncher (say warmUp, onSentToBackground, setInForeground,
> onBrougtToForeground) would cause a crash in debug builds.
> 
> Is that your intention? If it is, then I think that is fine with me. Just
wanted
> to check.
> 
> On 2014/10/23 22:57:26, Feng Qian wrote:
> > ChildProcessLauncher is called in browser process and CommandLine was
> > initialized already.
> > 
> > The assert here is to catch and prevent the case isLowEndDevice from being
> > called but command line wasn't initialized.
> > 
> > The try bot doesn't break tests, so I assume existing code wasn't broken by
> this
> > CL, and future violation should be caught by new assertion.
> > 
> > 
> > On 2014/10/23 22:30:48, nyquist wrote:
> > > How does this work for the ChildProcessLauncher?
> > > 
> > > The static member field
> > > org.chromium.content.browser.ChildProcessLauncher#sBindingManager is
> > statically
> > > initialized though this call hierarchy:
> > > org.chromium.content.browser.BindingManagerImpl#createBindingManager
> > > org.chromium.base.SysUtils#isLowEndDevice
> > > org.chromium.base.SysUtils#detectLowEndDevice
> > > 
> > > Are we sure that static initializer is never run before the CommandLine
has
> > been
> > > initialized?
> >

[nyquist, 2014-10-27 22:54:07]

lgtm

[Feng Qian, 2014-10-28 00:31:57]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-10-28 00:33:06]

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

[commit-bot: I haz the power, 2014-10-28 07:02:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-28 07:03:49]

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

[David Trainor- moved to gerrit, 2014-10-28 20:01:22]

On 2014/10/28 07:03:49, I haz the power (commit-bot) wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/0dfe9ba256030d15cc897295c3fa3bdd5d564404
> Cr-Commit-Position: refs/heads/master@{#301581}

This is causing test failures for some prerendering that checks isLowEndDevice
(see PrerenderServiceTest).