Fix CommandLine initialization for Chrome & WebView

Fix CommandLine initialization for Chrome & WebView

Chrome may access the CommandLine from Java in-between 'load' and
'initialize' which currently causes an crash since JNI is not ready
until after initialize but the CommandLine is switched over to native
after load (we moved the CommandLine switch to after load to fix a
problem in the Webview).

This fixes the problem for Chrome by moving the CommandLine switch
back to after initialize but provides a method to cause the switch to
happen early so the WebView problem can be fixed to.

BUG=417053, 331424
Committed: https://crrev.com/6516122e8e02e2d77f1470cb19330f62eeee3521
Cr-Commit-Position: refs/heads/master@{#296924}

[hjd, 2014-09-25 12:04:03]

hjd@chromium.org changed reviewers:
+ mkosiba@chromium.org, phajdan.jr@chromium.org

[hjd, 2014-09-25 12:04:03]

ptal

[hjd, 2014-09-25 12:04:16]

hjd@chromium.org changed reviewers:
- phajdan.jr@chromium.org

[hjd, 2014-09-25 12:37:57]

hjd@chromium.org changed reviewers:
+ aberent@chromium.org

[hjd, 2014-09-25 12:37:58]

ptal

[aberent, 2014-09-25 12:56:10]

https://codereview.chromium.org/598363003/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/598363003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:252:
assert !sLoaded;
This looks wrong. Do you really want this to happen before the library is
loaded? Should it be 'assert sLoaded'?

See also my comment on line 278.

https://codereview.chromium.org/598363003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:278:
ensureCommandLineSwitchedAlreadyLocked();
Why have you moved this from loadAlreadyLocked; it seems you could simply call
this function in loadAlreadyLocked, replacing the current code? Also, at least
in ensureInitialize, loadAlreadyLocked is called before initializeAlreadyLocked,
so sLoaded will be true, hence hitting the assert in
ensureCommandLineSwitchedAlreadyLocked.

[hjd, 2014-09-25 13:32:14]

https://codereview.chromium.org/598363003/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/598363003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:252:
assert !sLoaded;
On 2014/09/25 12:56:10, aberent wrote:
> This looks wrong. Do you really want this to happen before the library is
> loaded? Should it be 'assert sLoaded'?
> 
> See also my comment on line 278.

Done. Silly copy and paste error thanks :)

https://codereview.chromium.org/598363003/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:278:
ensureCommandLineSwitchedAlreadyLocked();
On 2014/09/25 12:56:10, aberent wrote:
> Why have you moved this from loadAlreadyLocked; it seems you could simply call
> this function in loadAlreadyLocked, replacing the current code? Also, at least
> in ensureInitialize, loadAlreadyLocked is called before
initializeAlreadyLocked,
> so sLoaded will be true, hence hitting the assert in
> ensureCommandLineSwitchedAlreadyLocked.

The fact that we're doing the CommandLine switch in loadAlreadyLocked
is the problem since in Chrome between loadAlreadyLocked and
initializeAlreadyLocked the JNI is not setup so calls to the Java command line
cause a crash sometimes (since after the swap the Java CommandLine delegates to
native so if we happen to call CommandLine.hasFlag or something between
loadAlreadyLocked and initializeAlreadyLocked we crash).

So we want to move the CommandLine switch over back to after
initializeAlreadyLocked (notwithstanding the silly assert typo) where it came
from originally to fix the problem.

[aberent, 2014-09-25 13:38:06]

lgtm

[aberent, 2014-09-25 13:38:39]

On 2014/09/25 13:38:06, aberent wrote:
> lgtm

That should have been:

lgtm once the assert is fixed.

[mkosiba (inactive), 2014-09-25 14:40:43]

lgtm

[hjd, 2014-09-25 18:14:56]

hjd@chromium.org changed reviewers:
+ fqian@chromium.org, klobag@chromium.org, mariakhomenko@chromium.org
- aberent@chromium.org

[hjd, 2014-09-25 18:14:57]

ptal, if this fixes the crbug.com/417053 feel free to land.

[Feng Qian, 2014-09-25 18:42:38]

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

[Feng Qian, 2014-09-25 18:42:39]

https://codereview.chromium.org/598363003/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/598363003/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:243:
synchronized (sLock) {
"JNI is already loaded by this point", what's expected value of sInitialized at
this point?

Should we inline ensureCommandLineSwitchedAlreadyLocked here?

[hjd_google, 2014-09-25 18:50:51]

hjd@google.com changed reviewers:
+ hjd@google.com

[hjd_google, 2014-09-25 18:50:52]

https://codereview.chromium.org/598363003/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/598363003/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:243:
synchronized (sLock) {
On 2014/09/25 18:42:38, Feng Qian wrote:
> "JNI is already loaded by this point", what's expected value of sInitialized
at
> this point?
False

> Should we inline ensureCommandLineSwitchedAlreadyLocked here?
I thought this was slightly clearer but I'm fine with that.

[chromium-reviews, 2014-09-25 19:15:38]

This will probably break:

https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

Which will need to updated to call switchCommandLineForWebView / removed
for now.
On 25 Sep 2014 19:50, <hjd@google.com> wrote:

>
> https://codereview.chromium.org/598363003/diff/80001/base/
> android/java/src/org/chromium/base/library_loader/LibraryLoader.java
> File
> base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
> (right):
>
> https://codereview.chromium.org/598363003/diff/80001/base/
> android/java/src/org/chromium/base/library_loader/
> LibraryLoader.java#newcode243
> base/android/java/src/org/chromium/base/library_loader/
> LibraryLoader.java:243:
> synchronized (sLock) {
> On 2014/09/25 18:42:38, Feng Qian wrote:
>
>> "JNI is already loaded by this point", what's expected value of
>>
> sInitialized at
>
>> this point?
>>
> False
>
>  Should we inline ensureCommandLineSwitchedAlreadyLocked here?
>>
> I thought this was slightly clearer but I'm fine with that.
>
> https://codereview.chromium.org/598363003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Feng Qian, 2014-09-25 21:22:48]

BTW, we can reproduce the issue by introduce a wait between 
        parts.setContentViewAndLoadLibrary();
        postInflationStartup();

My opinion is to revert the original CL if possible, and re-make
a clean fix for webview and Clank.

https://codereview.chromium.org/598363003/diff/80001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/598363003/diff/80001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:268:
if (!sCommandLineSwitched) {
Changes in initializeAlreadyLocked fix Chrome crash.
I am fine with this part of both CLs.

You just need to handle webview-side properly, and commit it.

Please request a merge into M38 branch.

[chromium-reviews, 2014-09-25 21:25:43]

Also, you can now (if you sync today) just build and run Chrome.apk on an
L-phone and you will get Hera by default.

On Thu, Sep 25, 2014 at 2:22 PM, <feng@chromium.org> wrote:

> BTW, we can reproduce the issue by introduce a wait between
>         parts.setContentViewAndLoadLibrary();
>         postInflationStartup();
>
> My opinion is to revert the original CL if possible, and re-make
> a clean fix for webview and Clank.
>
>
> https://codereview.chromium.org/598363003/diff/80001/base/
> android/java/src/org/chromium/base/library_loader/LibraryLoader.java
> File
> base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
> (right):
>
> https://codereview.chromium.org/598363003/diff/80001/base/
> android/java/src/org/chromium/base/library_loader/
> LibraryLoader.java#newcode268
> base/android/java/src/org/chromium/base/library_loader/
> LibraryLoader.java:268:
> if (!sCommandLineSwitched) {
> Changes in initializeAlreadyLocked fix Chrome crash.
> I am fine with this part of both CLs.
>
> You just need to handle webview-side properly, and commit it.
>
> Please request a merge into M38 branch.
>
> https://codereview.chromium.org/598363003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-09-25 21:27:00]

Oh, and also it reproduces on KitKat as well, so Chrome on KitKat is all
you really need to reproduce with Feng's suggestion.

On Thu, Sep 25, 2014 at 2:25 PM, Maria Khomenko <mariakhomenko@google.com>
wrote:

> Also, you can now (if you sync today) just build and run Chrome.apk on an
> L-phone and you will get Hera by default.
>
> On Thu, Sep 25, 2014 at 2:22 PM, <feng@chromium.org> wrote:
>
>> BTW, we can reproduce the issue by introduce a wait between
>>         parts.setContentViewAndLoadLibrary();
>>         postInflationStartup();
>>
>> My opinion is to revert the original CL if possible, and re-make
>> a clean fix for webview and Clank.
>>
>>
>> https://codereview.chromium.org/598363003/diff/80001/base/
>> android/java/src/org/chromium/base/library_loader/LibraryLoader.java
>> File
>> base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
>> (right):
>>
>> https://codereview.chromium.org/598363003/diff/80001/base/
>> android/java/src/org/chromium/base/library_loader/
>> LibraryLoader.java#newcode268
>> base/android/java/src/org/chromium/base/library_loader/
>> LibraryLoader.java:268:
>> if (!sCommandLineSwitched) {
>> Changes in initializeAlreadyLocked fix Chrome crash.
>> I am fine with this part of both CLs.
>>
>> You just need to handle webview-side properly, and commit it.
>>
>> Please request a merge into M38 branch.
>>
>> https://codereview.chromium.org/598363003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-09-25 21:34:45]

Changes that can reproduce the crash, for your debugging purpose.

https://codereview.chromium.org/601423003
https://chrome-internal-review.googlesource.com/177567

On Thu, Sep 25, 2014 at 2:26 PM, Maria Khomenko <mariakhomenko@google.com>
wrote:

> Oh, and also it reproduces on KitKat as well, so Chrome on KitKat is all
> you really need to reproduce with Feng's suggestion.
>
> On Thu, Sep 25, 2014 at 2:25 PM, Maria Khomenko <mariakhomenko@google.com>
> wrote:
>
>> Also, you can now (if you sync today) just build and run Chrome.apk on an
>> L-phone and you will get Hera by default.
>>
>> On Thu, Sep 25, 2014 at 2:22 PM, <feng@chromium.org> wrote:
>>
>>> BTW, we can reproduce the issue by introduce a wait between
>>>         parts.setContentViewAndLoadLibrary();
>>>         postInflationStartup();
>>>
>>> My opinion is to revert the original CL if possible, and re-make
>>> a clean fix for webview and Clank.
>>>
>>>
>>> https://codereview.chromium.org/598363003/diff/80001/base/
>>> android/java/src/org/chromium/base/library_loader/LibraryLoader.java
>>> File
>>> base/android/java/src/org/chromium/base/library_loader/
>>> LibraryLoader.java
>>> (right):
>>>
>>> https://codereview.chromium.org/598363003/diff/80001/base/
>>> android/java/src/org/chromium/base/library_loader/
>>> LibraryLoader.java#newcode268
>>> base/android/java/src/org/chromium/base/library_loader/
>>> LibraryLoader.java:268:
>>> if (!sCommandLineSwitched) {
>>> Changes in initializeAlreadyLocked fix Chrome crash.
>>> I am fine with this part of both CLs.
>>>
>>> You just need to handle webview-side properly, and commit it.
>>>
>>> Please request a merge into M38 branch.
>>>
>>> https://codereview.chromium.org/598363003/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hjd, 2014-09-26 10:54:27]

The CQ bit was checked by hjd@chromium.org

[commit-bot: I haz the power, 2014-09-26 10:54:58]

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

[commit-bot: I haz the power, 2014-09-26 11:01:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-26 11:01:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hjd, 2014-09-26 11:03:35]

hjd@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[hjd, 2014-09-26 11:03:36]

Ross could you ptal at LibraryLoader.java for an OWNERS review?

Thanks! :)

[rmcilroy, 2014-09-26 11:36:33]

On 2014/09/26 11:03:36, hjd wrote:
> Ross could you ptal at LibraryLoader.java for an OWNERS review?
> 
> Thanks! :)

lgtm, thanks!

[hjd, 2014-09-26 12:21:07]

The CQ bit was checked by hjd@chromium.org

[commit-bot: I haz the power, 2014-09-26 12:22:42]

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

[commit-bot: I haz the power, 2014-09-26 12:26:50]

Committed patchset #6 (id:100001) as 6190a213faeb2a2b9e4a9a98776aaf9e85b2b2ee

[commit-bot: I haz the power, 2014-09-26 12:27:32]

Patchset 6 (id:??) landed as
https://crrev.com/6516122e8e02e2d77f1470cb19330f62eeee3521
Cr-Commit-Position: refs/heads/master@{#296924}