[Android] Workaround of an android platform bug.

[Android] Workaround of an android platform bug.

On some Android devices (e.g., Sony Xperia), package manager may
fail to extract native libraries when updating Chrome.

The change tries alleviate the situation by:
1) name libchrome with version number;
2) when failed to load library, try to extract native libraies
   and load them.

BUG=311644
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258546

[Feng Qian, 2014-03-14 19:14:50]

Sorry guys,

This is another try to workaround issue 311644.
The cause of problem is different from what we thought.
The patch description is more accurate.

Instead of piggybacking on the old CL, I created this new CL.

Downstream CL will be updated as well.

[Feng Qian, 2014-03-14 19:15:29]

oops, forgot to include the prev CL link:
https://codereview.chromium.org/180273005/

On 2014/03/14 19:14:50, Feng Qian wrote:
> Sorry guys,
> 
> This is another try to workaround issue 311644.
> The cause of problem is different from what we thought.
> The patch description is more accurate.
> 
> Instead of piggybacking on the old CL, I created this new CL.
> 
> Downstream CL will be updated as well.

[cjhopman, 2014-03-17 18:32:35]

https://codereview.chromium.org/200753002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/200753002/diff/20001/build/common.gypi#newcod...
build/common.gypi:3783: 'android_product_extension': '<(lib_version).so',
This is going to apply to all shared libraries. We only need this for libchrome,
so why not only set it for that library?

https://codereview.chromium.org/200753002/diff/20001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/200753002/diff/20001/build/java_apk.gypi#newc...
build/java_apk.gypi:187: ['native_lib_target != "" and component !=
"shared_library" and use_chromium_linker != 1', {
I think this would be better if it just explicitly listed the libraries to keep
and then deleted everything else.

Also, this shouldn't be disabled when use_chromium_linker == 1 (since
use_chromium_linker==1 for chrome_apk).

[Feng Qian, 2014-03-18 00:06:59]

https://codereview.chromium.org/200753002/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/200753002/diff/20001/build/common.gypi#newcod...
build/common.gypi:3783: 'android_product_extension': '<(lib_version).so',
Strictly speaking, it applies to all libraries only in non-component build.
I can try to see how difficult to only change libchrome naming.

On 2014/03/17 18:32:35, cjhopman wrote:
> This is going to apply to all shared libraries. We only need this for
libchrome,
> so why not only set it for that library?

https://codereview.chromium.org/200753002/diff/20001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/200753002/diff/20001/build/java_apk.gypi#newc...
build/java_apk.gypi:187: ['native_lib_target != "" and component !=
"shared_library" and use_chromium_linker != 1', {
Listing libraries to keep will have issues with building multiple targets, if
the list only has libraries of current target, it will delete libraries of other
targets.

I am not sure if it will work.

Removing 'use_chromium_linker' check. It reminds me that I forgot to include
changes in Linker.java :)

On 2014/03/17 18:32:35, cjhopman wrote:
> I think this would be better if it just explicitly listed the libraries to
keep
> and then deleted everything else.
> 
> Also, this shouldn't be disabled when use_chromium_linker == 1 (since
> use_chromium_linker==1 for chrome_apk).

[cjhopman, 2014-03-18 00:16:54]

https://codereview.chromium.org/200753002/diff/20001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/200753002/diff/20001/build/java_apk.gypi#newc...
build/java_apk.gypi:187: ['native_lib_target != "" and component !=
"shared_library" and use_chromium_linker != 1', {
On 2014/03/18 00:07:01, Feng Qian wrote:
> Listing libraries to keep will have issues with building multiple targets, if
> the list only has libraries of current target, it will delete libraries of
other
> targets.
> 
> I am not sure if it will work.
> 
> Removing 'use_chromium_linker' check. It reminds me that I forgot to include
> changes in Linker.java :)
> 
> On 2014/03/17 18:32:35, cjhopman wrote:
> > I think this would be better if it just explicitly listed the libraries to
> keep
> > and then deleted everything else.
> > 
> > Also, this shouldn't be disabled when use_chromium_linker == 1 (since
> > use_chromium_linker==1 for chrome_apk).
> 
Oh right, because this is deleting the ones in <(SHARED_LIB_DIR) too. How about,
along with making this only affect libchromeview we also move the action into
that target or the chrome_apk target (i.e. it shouldn't really be needed for all
apks).

[klobag.chromium, 2014-03-18 19:17:23]

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:55:
*  If named library is not in backup directory, extract native libraries
can you clarify what is the backup directory?

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:75:
private static File getWorkaroundLibFile(Context context, String library) {
is this return the lib under the /data/data/<pack>?

https://codereview.chromium.org/200753002/diff/40001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentCommandLineTest.java
(right):

https://codereview.chromium.org/200753002/diff/40001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentCommandLineTest.java:51:
getInstrumentation().getTargetContext(), false);
Should this be true?

[Feng Qian, 2014-03-18 21:59:27]

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:55:
*  If named library is not in backup directory, extract native libraries
On 2014/03/18 19:17:24, klobag.chromium wrote:
> can you clarify what is the backup directory?

Done.

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:75:
private static File getWorkaroundLibFile(Context context, String library) {
On 2014/03/18 19:17:24, klobag.chromium wrote:
> is this return the lib under the /data/data/<pack>?
yes, context.getDir(LIB_DIR, ...) returns
/data/data/<pkg>/app_lib

https://codereview.chromium.org/200753002/diff/40001/content/public/android/j...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentCommandLineTest.java
(right):

https://codereview.chromium.org/200753002/diff/40001/content/public/android/j...
content/public/android/javatests/src/org/chromium/content/browser/ContentCommandLineTest.java:51:
getInstrumentation().getTargetContext(), false);
On 2014/03/18 19:17:24, klobag.chromium wrote:
> Should this be true?

This is a test, I think it is save to say 'false' here.

[klobag.chromium, 2014-03-19 00:32:27]

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

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:167:
LibraryLoaderHelper.cleanupOldLibraries(context, !sNativeLibraryHack);
Should we move this after line 132 and delete the dir? When we do unpack, we
delete dir first. It is easier to read.

[Feng Qian, 2014-03-19 16:08:26]

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

https://codereview.chromium.org/200753002/diff/40001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:167:
LibraryLoaderHelper.cleanupOldLibraries(context, !sNativeLibraryHack);
Sure, sounds a good plan.

On 2014/03/19 00:32:28, klobag.chromium wrote:
> Should we move this after line 132 and delete the dir? When we do unpack, we
> delete dir first. It is easier to read.

[Feng Qian, 2014-03-19 16:47:51]

Darin, the CL has small changes in mojo code, which requires mojo owner's
approval. Can you give a quick LGTM after others approved CL? Thanks in advance.

[klobag.chromium, 2014-03-19 17:23:45]

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:61:
public static void ensureInitialized(Context context, boolean isBrowserProcess)
can we change the name of isBrowserProcess? As in WebView and test, you pass
false while they are all main process.

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:96:
file.close();
should we call deleteDirectorySync(libDir) to cleanup if error happened?

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:138:
Log.e(TAG, "Failed to unpack native libraries", e);
clean up?

https://codereview.chromium.org/200753002/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/200753002/diff/100001/build/common.gypi#newco...
build/common.gypi:2049: 'component%': '<(component)',
why need this?

[Feng Qian, 2014-03-19 17:52:50]

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:61:
public static void ensureInitialized(Context context, boolean isBrowserProcess)
Per discussion, rename to 'shouldApplyNativeLibraryHack'.

On 2014/03/19 17:23:46, klobag.chromium wrote:
> can we change the name of isBrowserProcess? As in WebView and test, you pass
> false while they are all main process.

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:96:
file.close();
On 2014/03/19 17:23:46, klobag.chromium wrote:
> should we call deleteDirectorySync(libDir) to cleanup if error happened?

Done.

https://codereview.chromium.org/200753002/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:138:
Log.e(TAG, "Failed to unpack native libraries", e);
On 2014/03/19 17:23:46, klobag.chromium wrote:
> clean up?

Done.

https://codereview.chromium.org/200753002/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/200753002/diff/100001/build/common.gypi#newco...
build/common.gypi:2049: 'component%': '<(component)',
On 2014/03/19 17:23:46, klobag.chromium wrote:
> why need this?

gyp target_conditions has its own variable scope. Without definition here, we
cannot use 'component' in target_conditions. See clank-side change, line 95:
https://chrome-internal-review.googlesource.com/#/c/156166/8/native/framework...

[Yaron, 2014-03-20 01:24:48]

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:48:
public static boolean sNativeLibraryHack = false;
nit: sNativeLibraryHackWasUsed (to reflect that this is after the fact)

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:144:
if (LibraryLoaderHelper.tryLoadLibraryUsingWorkaround(
I'm not sure about this but have you considered catching Exception here and
wrapping it in ProcessInitException? If you did that, chrome would throw up a
user prompt saying installation is broken and try uninstalling-reinstalling
which might help some users.

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:145:
context, library, useNativeLibraryHack)) {
nit: indent 4 more.

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:115:
deleteDirectorySync(libDir);
If you did have multiple libraries to load this would break on the second
invocation. Can you delete earlier?

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:121:
for (String libName : NativeLibraries.LIBRARIES) {
Why are you iterating through NativeLibraries.LIBRARIES again? The caller is
doing that, just pass |library| into here.

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:148:
byte[] buffer = new byte[4096];
How'd you choose 4k? ResourceExtractor uses 16K

[Feng Qian, 2014-03-20 15:28:47]

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:48:
public static boolean sNativeLibraryHack = false;
Good suggestion, done.

On 2014/03/20 01:24:48, Yaron wrote:
> nit: sNativeLibraryHackWasUsed (to reflect that this is after the fact)

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:144:
if (LibraryLoaderHelper.tryLoadLibraryUsingWorkaround(
I think the code is doing what you suggest. Line 162 catches the
UnsatisfiedLinkError and wraps it into a ProcessInitException. Do I miss
something?

On 2014/03/20 01:24:48, Yaron wrote:
> I'm not sure about this but have you considered catching Exception here and
> wrapping it in ProcessInitException? If you did that, chrome would throw up a
> user prompt saying installation is broken and try uninstalling-reinstalling
> which might help some users.

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:145:
context, library, useNativeLibraryHack)) {
On 2014/03/20 01:24:48, Yaron wrote:
> nit: indent 4 more.

Done.

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:115:
deleteDirectorySync(libDir);
Unpacking should only be done once. Line 69 checks the existence of lib file,
and second library loading should succeed. Otherwise, it is a mismatching
between native libs in APK and NativeLibraries.LIBRARIES.

I can add a boolean check to make sure unpackLibraries is only called once.

On 2014/03/20 01:24:48, Yaron wrote:
> If you did have multiple libraries to load this would break on the second
> invocation. Can you delete earlier?

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:121:
for (String libName : NativeLibraries.LIBRARIES) {
Logic is to call unpacking all libraries only on the first library loading.

On 2014/03/20 01:24:48, Yaron wrote:
> Why are you iterating through NativeLibraries.LIBRARIES again? The caller is
> doing that, just pass |library| into here.

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:148:
byte[] buffer = new byte[4096];
4k was copied from Ptts fix, 16k is fine.
A good question is why choose 16k? ;)

On 2014/03/20 01:24:48, Yaron wrote:
> How'd you choose 4k? ResourceExtractor uses 16K

[klobag.chromium, 2014-03-20 16:26:44]

https://codereview.chromium.org/200753002/diff/140001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/140001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:127:
if (sLibrariesWereUnpacked) {
hmm, will this cause loadNativeLibrariesUsingWorkaroundForTesting() fail if
there is more than one in NativeLibraries.LIBRARIES?

Also the line 141 already traverse NativeLibraries.LIBRARIES.

[klobag.chromium, 2014-03-20 16:55:22]

https://codereview.chromium.org/200753002/diff/160001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/160001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:43:
*/
add @VisibleForTesting, then you don't need to make it public.

[klobag.chromium, 2014-03-20 17:01:12]

lgtm

[sky, 2014-03-20 17:21:45]

mojo/shell/android/apk/src/org/chromium/mojo_shell_apk/MojoShellActivity.java
LGTM

[Feng Qian, 2014-03-20 17:46:08]

The CQ bit was checked by feng@chromium.org

[Yaron, 2014-03-20 17:48:44]

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:144:
if (LibraryLoaderHelper.tryLoadLibraryUsingWorkaround(
On 2014/03/20 15:28:47, Feng Qian wrote:
> I think the code is doing what you suggest. Line 162 catches the
> UnsatisfiedLinkError and wraps it into a ProcessInitException. Do I miss
> something?
> 
That it's catching *only* UnsatisfiedLinkError. :)
You're now running a bunch of logic here to try and load the library but the
crux of it is to try a hack to get the library to load. This issue could be
resolved by uninstall/reinstall (I think?) in which case helping the user with a
prompt is better than a permanent random crash. In general I don't want to catch
all exceptions, but I think it may be warranted here. Open to not doing it, just
want to make it clear what I'm thinking.

> On 2014/03/20 01:24:48, Yaron wrote:
> > I'm not sure about this but have you considered catching Exception here and
> > wrapping it in ProcessInitException? If you did that, chrome would throw up
a
> > user prompt saying installation is broken and try uninstalling-reinstalling
> > which might help some users.
>

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:148:
byte[] buffer = new byte[4096];
On 2014/03/20 15:28:47, Feng Qian wrote:
> 4k was copied from Ptts fix, 16k is fine.
> A good question is why choose 16k? ;)
> 
> On 2014/03/20 01:24:48, Yaron wrote:
> > How'd you choose 4k? ResourceExtractor uses 16K
> 

Heh. I dunno in this instance

[commit-bot: I haz the power, 2014-03-20 17:50:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/180001

[Yaron, 2014-03-20 17:51:54]

https://codereview.chromium.org/200753002/diff/180001/base/android/java/src/o...
File
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java
(right):

https://codereview.chromium.org/200753002/diff/180001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoaderHelper.java:131:
return false;
This doesn't seem right

You want L93 to proceed and not return early if unpacking happened correctly for
a previous library. This should be indicating that, yes, we have unpacked the
libraries. However, you're automatically setting it to true afterwards and it's
not based on whether unpacking was successful.

[Yaron, 2014-03-20 18:11:47]

lgtm based on off-thread explanation. thanks

[commit-bot: I haz the power, 2014-03-20 18:31:39]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Yaron, 2014-03-20 18:34:46]

lgtm

https://codereview.chromium.org/200753002/diff/200001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/200753002/diff/200001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:48:
public static boolean sNativeLibraryHackWasUsed = false;
Does this still need to be public?

https://codereview.chromium.org/200753002/diff/200001/base/android/library_lo...
File base/android/library_loader/library_loader_hooks.cc (right):

https://codereview.chromium.org/200753002/diff/200001/base/android/library_lo...
base/android/library_loader/library_loader_hooks.cc:68:
UMA_HISTOGRAM_BOOLEAN("MobileStart.NativeLibraryHack", usedHack);
I'm fine with this but could be named LibraryLoader.*

[Feng Qian, 2014-03-20 18:36:09]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:40:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/200001

[Feng Qian, 2014-03-20 18:43:05]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:55:27]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-03-20 18:55:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/220001

[commit-bot: I haz the power, 2014-03-20 19:25:58]

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

[commit-bot: I haz the power, 2014-03-20 19:25:59]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[michaelbai, 2014-03-20 20:38:23]

lgtm

[Feng Qian, 2014-03-20 20:38:51]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:39:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/220001

[commit-bot: I haz the power, 2014-03-20 22:36:38]

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

[commit-bot: I haz the power, 2014-03-20 22:36:39]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[Feng Qian, 2014-03-20 23:03:55]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-03-20 23:04:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/240001

[boliu, 2014-03-20 23:27:11]

lgtm since I don't want to block cq...

https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
(right):

https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:37:
LibraryLoader.loadNow(null, false);
This will never have a context. This is called inside the zygote process of
android, at which point it can't possibly know what app it is hosting.

https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:104:
BrowserStartupController.get(context).startBrowserProcessesSync(
Hmm...why does this need a context...

https://codereview.chromium.org/200753002/diff/240001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/200753002/diff/240001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:145:
if (LibraryLoaderHelper.tryLoadLibraryUsingWorkaround(
Hmm, this will NPE with null context, but I guess that's no different than
throwing a link error.

[commit-bot: I haz the power, 2014-03-20 23:36:11]

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

[commit-bot: I haz the power, 2014-03-20 23:36:12]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[boliu, 2014-03-20 23:50:11]

https://codereview.chromium.org/200753002/diff/240001/build/android/gyp/delet...
File build/android/gyp/delete_files.py (right):

https://codereview.chromium.org/200753002/diff/240001/build/android/gyp/delet...
build/android/gyp/delete_files.py:12: import shutil
I think presubmit is complaining you don't need this import.

And curious, where is this python script used? Seems out of place for this cl.

[Feng Qian, 2014-03-20 23:59:48]

I changed the API to loadLibrary(Context context), and it accepts
null context. In the case of null context, it doesn't use it.

On 2014/03/20 23:27:11, boliu wrote:
> lgtm since I don't want to block cq...
> 
>
https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
> File
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
> (right):
> 
>
https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:37:
> LibraryLoader.loadNow(null, false);
> This will never have a context. This is called inside the zygote process of
> android, at which point it can't possibly know what app it is hosting.
> 
>
https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:104:
> BrowserStartupController.get(context).startBrowserProcessesSync(
> Hmm...why does this need a context...
> 
>
https://codereview.chromium.org/200753002/diff/240001/base/android/java/src/o...
> File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
> (right):
> 
>
https://codereview.chromium.org/200753002/diff/240001/base/android/java/src/o...
> base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:145:
> if (LibraryLoaderHelper.tryLoadLibraryUsingWorkaround(
> Hmm, this will NPE with null context, but I guess that's no different than
> throwing a link error.

[Feng Qian, 2014-03-21 00:01:41]

https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
(right):

https://codereview.chromium.org/200753002/diff/240001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:37:
LibraryLoader.loadNow(null, false);
ok, will make it LibraryLoader.loadNow(null), and the method will handle
the null context properly.

On 2014/03/20 23:27:12, boliu wrote:
> This will never have a context. This is called inside the zygote process of
> android, at which point it can't possibly know what app it is hosting.

[Feng Qian, 2014-03-21 00:17:43]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/280001

[commit-bot: I haz the power, 2014-03-21 00:23:50]

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

[commit-bot: I haz the power, 2014-03-21 00:23:50]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[Feng Qian, 2014-03-21 00:27:57]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:30:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/280001

[commit-bot: I haz the power, 2014-03-21 00:34:45]

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

[commit-bot: I haz the power, 2014-03-21 00:34:46]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[Feng Qian, 2014-03-21 00:49:47]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:51:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/feng@chromium.org/200753002/280001

[commit-bot: I haz the power, 2014-03-21 12:39:58]

Change committed as 258546

[Hezley Khez, 2015-11-13 18:55:12]

hezley4sus@gmail.com changed reviewers:
+ hezley4sus@gmail.com

[Hezley Khez, 2015-11-13 18:55:13]

Issues 200753002 [Android] workaround of an android platform bug