Re-invent page aligning libraries in APK file.

Re-invent page aligning libraries in APK file.

With this change setting 'load_library_from_zip_file' to 1 will now
construct the APK with a new tool chain. The purpose of this change is
to contruct the APK in such a way that the page alignment is not broken
by the chrome signing process. To achieve this the tool is written in
Java so it shares the same underlying JarOutputStream implementation.
The key things that we depend on are that the filenames are output in
lexographical order with the META_INF files at the end of the file.
That zipalign adds 4 byte alignment to STORED files and this is done by
adding padding to the zip extra field.

BUG=390618
Committed: https://crrev.com/c83a88d39329e2a7d8bbc1de280d9e572d2f7c8a
Cr-Commit-Position: refs/heads/master@{#297439}

[Anton, 2014-09-26 14:08:27]

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

[rmcilroy, 2014-09-29 11:38:33]

First pass.  Looks good overall.

https://codereview.chromium.org/595933003/diff/1/build/android/gyp/finalize_a...
File build/android/gyp/finalize_apk.py (right):

https://codereview.chromium.org/595933003/diff/1/build/android/gyp/finalize_a...
build/android/gyp/finalize_apk.py:126: ReorderApk(options.prealign_path,
signed_apk_path, options.final_apk_path)
Given that ReorderApk also aligns the APK, could you mention that in the comment
above and name this function ReorderAndAlignApk, just so nobody is tempted to do
the AlignApk step on the output of this step.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
File build/android/rezip/PrealignApk.java (right):

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:26: class PrealignApk {
I'm not sure Prealign is a very informative name for what this tool does (it
seems to be used both at the start and end of the process).  How about RezipApk
or similar?  

Also, any chance we could re-implement the bits required in rezip in Java and do
them here to avoid having two tools to perform the one process (not necessarily
in this CL, I would just like an idea of the feasibility)?

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:94: private static List<String>
orderFilenames(JarFile jar, boolean addAlignment) {
/s/addAlignment/omitMetaFiles

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:255: offset, timestamp, name, prevName,
out, addAlignment);
nit - could we just do the addAlignmentFile method call "if (addAlignment)"? The
only think addAlignmentFile does when addAlignment is false is the alignment
check, so you could probably  do that explicitly here (as you do below on line
258...) unconditionally.  WDYT?

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:266: } else if (ALIGNMENT > 0) {
is it worth having this check given ALIGNMENT is always 4?

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:290: System.err.println("  reorder      -
re-creates canonical ordering checks alignment");
/s/re-creates canonical ordering checks alignment/re-creates canonical ordering
and checks alignment

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:313: inputJar = new JarFile(new
File(inputFilename), false);  // Don't verify.
Is there a reason not to verify? If so, could you add the reason to the comment
please.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:326: timestamp, addAlignment);
nit - the "List<String> names" and "timestamp" arguments can be created inside
the copyAndAlignFiles method as locals using the information available to it -
please move them to be locals in copyAndAlignFiles.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/rezip.cc
File build/android/rezip/rezip.cc (right):

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/rezip.cc...
build/android/rezip/rezip.cc:512: inflate_predicate_fun = &IsLibraryFilename;
It looks like this is the only option used by the build system any longer. 
Could you remove the other options and just make rezip do this by default?

https://codereview.chromium.org/595933003/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/595933003/diff/1/build/java_apk.gypi#newcode501
build/java_apk.gypi:501: '<(DEPTH)/build/android/rezip.gyp:prealign',
you only need this for host right? (e.g., ...prealign#host)?

https://codereview.chromium.org/595933003/diff/1/build/java_apk.gypi#newcode537
build/java_apk.gypi:537: '<(DEPTH)/build/android/rezip.gyp:prealign',
ditto

[Anton, 2014-09-29 16:30:18]

https://codereview.chromium.org/595933003/diff/1/build/android/gyp/finalize_a...
File build/android/gyp/finalize_apk.py (right):

https://codereview.chromium.org/595933003/diff/1/build/android/gyp/finalize_a...
build/android/gyp/finalize_apk.py:126: ReorderApk(options.prealign_path,
signed_apk_path, options.final_apk_path)
On 2014/09/29 11:38:32, rmcilroy wrote:
> Given that ReorderApk also aligns the APK, could you mention that in the
comment
> above and name this function ReorderAndAlignApk, just so nobody is tempted to
do
> the AlignApk step on the output of this step.

Done.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
File build/android/rezip/PrealignApk.java (right):

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:26: class PrealignApk {
On 2014/09/29 11:38:32, rmcilroy wrote:
> I'm not sure Prealign is a very informative name for what this tool does (it
> seems to be used both at the start and end of the process).  How about
RezipApk
> or similar?  

Done

> Also, any chance we could re-implement the bits required in rezip in Java and
do
> them here to avoid having two tools to perform the one process (not
necessarily
> in this CL, I would just like an idea of the feasibility)?

Yeah, I thought about this, but it doesn't seem worth it as we don't get rid of
"rezip" this way, because it is still needed for the LGPL compliance test. I
don't fancy making the LGPL compliance instructions require that you install
java and so forth. "rezip" is sufficient since SignApk is not used in this flow.

The performance of the finalize APK build step is now dominated by the fact that
we have to recompress all the files 3 times (because of the way the Java library
work). So getting rid of rezip doesn't help here either.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:94: private static List<String>
orderFilenames(JarFile jar, boolean addAlignment) {
On 2014/09/29 11:38:32, rmcilroy wrote:
> /s/addAlignment/omitMetaFiles

Done.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:255: offset, timestamp, name, prevName,
out, addAlignment);
On 2014/09/29 11:38:32, rmcilroy wrote:
> nit - could we just do the addAlignmentFile method call "if (addAlignment)"?
The
> only think addAlignmentFile does when addAlignment is false is the alignment
> check, so you could probably  do that explicitly here (as you do below on line
> 258...) unconditionally.  WDYT?

Done.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:266: } else if (ALIGNMENT > 0) {
On 2014/09/29 11:38:32, rmcilroy wrote:
> is it worth having this check given ALIGNMENT is always 4?

Removed. Compiler will take them out anyway, but they seem like clutter.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:290: System.err.println("  reorder      -
re-creates canonical ordering checks alignment");
On 2014/09/29 11:38:32, rmcilroy wrote:
> /s/re-creates canonical ordering checks alignment/re-creates canonical
ordering
> and checks alignment

Done.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:313: inputJar = new JarFile(new
File(inputFilename), false);  // Don't verify.
On 2014/09/29 11:38:32, rmcilroy wrote:
> Is there a reason not to verify? If so, could you add the reason to the
comment
> please.

I am not sure of the merits of verifying existing signatures during the signing
process. I was just doing what the other programs do. I have changed to true
which also works. If the APK has a broken signature we will reject it at this
point, rather than sign it afresh.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:326: timestamp, addAlignment);
On 2014/09/29 11:38:32, rmcilroy wrote:
> nit - the "List<String> names" and "timestamp" arguments can be created inside
> the copyAndAlignFiles method as locals using the information available to it -
> please move them to be locals in copyAndAlignFiles.

Done.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/rezip.cc
File build/android/rezip/rezip.cc (right):

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/rezip.cc...
build/android/rezip/rezip.cc:512: inflate_predicate_fun = &IsLibraryFilename;
On 2014/09/29 11:38:32, rmcilroy wrote:
> It looks like this is the only option used by the build system any longer. 
> Could you remove the other options and just make rezip do this by default?

They are used by the LGPL compliance step of the official build bot. Added a
comment so that no one else makes that mistake, since that things is a bit
invisible.

https://codereview.chromium.org/595933003/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/595933003/diff/1/build/java_apk.gypi#newcode501
build/java_apk.gypi:501: '<(DEPTH)/build/android/rezip.gyp:prealign',
On 2014/09/29 11:38:33, rmcilroy wrote:
> you only need this for host right? (e.g., ...prealign#host)?

prealign#host does not work because Java rules don't work the same way as C++
rules in GYP. I know because that is where I started.

https://codereview.chromium.org/595933003/diff/1/build/java_apk.gypi#newcode537
build/java_apk.gypi:537: '<(DEPTH)/build/android/rezip.gyp:prealign',
On 2014/09/29 11:38:33, rmcilroy wrote:
> ditto

Acknowledged.

[cjhopman, 2014-09-29 18:33:02]

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

[cjhopman, 2014-09-29 18:33:03]

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp
File build/android/rezip.gyp (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp#...
build/android/rezip.gyp:28: 'javac_jar_path':
'<(PRODUCT_DIR)/lib.java/rezip.jar',
this can probably use build/host_jar.gypi (it looks like you would have to make
"jar_path" or just "jar_name" overridable).

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp#...
build/android/rezip.gyp:30: 'sources': [
'sources' here doesn't do anything.

[rmcilroy, 2014-09-30 09:36:33]

lgtm once comments are addressed.

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
File build/android/rezip/PrealignApk.java (right):

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:26: class PrealignApk {
On 2014/09/29 16:30:17, Anton wrote:
> On 2014/09/29 11:38:32, rmcilroy wrote:
> > I'm not sure Prealign is a very informative name for what this tool does (it
> > seems to be used both at the start and end of the process).  How about
> RezipApk
> > or similar?  
> 
> Done
> 
> > Also, any chance we could re-implement the bits required in rezip in Java
and
> do
> > them here to avoid having two tools to perform the one process (not
> necessarily
> > in this CL, I would just like an idea of the feasibility)?
> 
> Yeah, I thought about this, but it doesn't seem worth it as we don't get rid
of
> "rezip" this way, because it is still needed for the LGPL compliance test. I
> don't fancy making the LGPL compliance instructions require that you install
> java and so forth. "rezip" is sufficient since SignApk is not used in this
flow.
> 
> The performance of the finalize APK build step is now dominated by the fact
that
> we have to recompress all the files 3 times (because of the way the Java
library
> work). So getting rid of rezip doesn't help here either.

I would prefer that there were only one tool (and I don't think it would be much
of an issue to require Java for the LGPL instructions since I'm pretty sure this
would be required to do anything useful on the APK anyway), but I'm fine with us
landing this as is just now.

https://codereview.chromium.org/595933003/diff/20001/build/android/finalize_a...
File build/android/finalize_apk_action.gypi (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/finalize_a...
build/android/finalize_apk_action.gypi:30: 'rezip_jar_path%':
'<(PRODUCT_DIR)/lib.java/rezip.jar'
rezip_jar_path and rezip_apk.jar

https://codereview.chromium.org/595933003/diff/20001/build/android/finalize_a...
build/android/finalize_apk_action.gypi:57: '--rezip-jar-path=<(rezip_jar_path)',
ditto

https://codereview.chromium.org/595933003/diff/20001/build/android/gyp/finali...
File build/android/gyp/finalize_apk.py (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/gyp/finali...
build/android/gyp/finalize_apk.py:81: parser.add_option('--rezip-jar-path',
help='Path to the RezipApk jar file.')
rezip-apk-jar-path

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp
File build/android/rezip.gyp (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp#...
build/android/rezip.gyp:23: 'target_name': 'rezip_jar',
rezip_apk_jar?

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
File build/android/rezip/RezipApk.java (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
build/android/rezip/RezipApk.java:133: JarOutputStream out) throws IOException {
Fix alignment

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
build/android/rezip/RezipApk.java:209: boolean addAlignment) throws IOException
{
alignment

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
build/android/rezip/RezipApk.java:316: System.exit(1);
Are these catches (and the one on 322-323) really necessary?  Won't an uncaught
exception have the same effect (stacktrace + non-zero exit code)?

[Anton, 2014-09-30 09:52:34]

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp
File build/android/rezip.gyp (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp#...
build/android/rezip.gyp:28: 'javac_jar_path':
'<(PRODUCT_DIR)/lib.java/rezip.jar',
On 2014/09/29 18:33:03, cjhopman wrote:
> this can probably use build/host_jar.gypi (it looks like you would have to
make
> "jar_path" or just "jar_name" overridable).

I tried this, but it doesn't work. Proguard fails, it appears as if GYP tries to
include rezip.jar in the target jar. Don't know why that would be, GYP is a bit
of a mystery.

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp#...
build/android/rezip.gyp:30: 'sources': [
On 2014/09/29 18:33:03, cjhopman wrote:
> 'sources' here doesn't do anything.

Removed.

[Anton, 2014-09-30 10:30:01]

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
File build/android/rezip/PrealignApk.java (right):

https://codereview.chromium.org/595933003/diff/1/build/android/rezip/Prealign...
build/android/rezip/PrealignApk.java:26: class PrealignApk {
On 2014/09/30 09:36:32, rmcilroy wrote:
> On 2014/09/29 16:30:17, Anton wrote:
> > On 2014/09/29 11:38:32, rmcilroy wrote:
> > > I'm not sure Prealign is a very informative name for what this tool does
(it
> > > seems to be used both at the start and end of the process).  How about
> > RezipApk
> > > or similar?  
> > 
> > Done
> > 
> > > Also, any chance we could re-implement the bits required in rezip in Java
> and
> > do
> > > them here to avoid having two tools to perform the one process (not
> > necessarily
> > > in this CL, I would just like an idea of the feasibility)?
> > 
> > Yeah, I thought about this, but it doesn't seem worth it as we don't get rid
> of
> > "rezip" this way, because it is still needed for the LGPL compliance test. I
> > don't fancy making the LGPL compliance instructions require that you install
> > java and so forth. "rezip" is sufficient since SignApk is not used in this
> flow.
> > 
> > The performance of the finalize APK build step is now dominated by the fact
> that
> > we have to recompress all the files 3 times (because of the way the Java
> library
> > work). So getting rid of rezip doesn't help here either.
> 
> I would prefer that there were only one tool (and I don't think it would be
much
> of an issue to require Java for the LGPL instructions since I'm pretty sure
this
> would be required to do anything useful on the APK anyway), but I'm fine with
us
> landing this as is just now.

Acknowledged.

https://codereview.chromium.org/595933003/diff/20001/build/android/finalize_a...
File build/android/finalize_apk_action.gypi (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/finalize_a...
build/android/finalize_apk_action.gypi:30: 'rezip_jar_path%':
'<(PRODUCT_DIR)/lib.java/rezip.jar'
On 2014/09/30 09:36:32, rmcilroy wrote:
> rezip_jar_path and rezip_apk.jar

Done. Though I think you mean rezip_apk_jar_path.

https://codereview.chromium.org/595933003/diff/20001/build/android/finalize_a...
build/android/finalize_apk_action.gypi:57: '--rezip-jar-path=<(rezip_jar_path)',
On 2014/09/30 09:36:32, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/595933003/diff/20001/build/android/gyp/finali...
File build/android/gyp/finalize_apk.py (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/gyp/finali...
build/android/gyp/finalize_apk.py:81: parser.add_option('--rezip-jar-path',
help='Path to the RezipApk jar file.')
On 2014/09/30 09:36:32, rmcilroy wrote:
> rezip-apk-jar-path

Done.

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp
File build/android/rezip.gyp (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip.gyp#...
build/android/rezip.gyp:23: 'target_name': 'rezip_jar',
On 2014/09/30 09:36:32, rmcilroy wrote:
> rezip_apk_jar?

Done.

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
File build/android/rezip/RezipApk.java (right):

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
build/android/rezip/RezipApk.java:133: JarOutputStream out) throws IOException {
On 2014/09/30 09:36:33, rmcilroy wrote:
> Fix alignment

Done.

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
build/android/rezip/RezipApk.java:209: boolean addAlignment) throws IOException
{
On 2014/09/30 09:36:33, rmcilroy wrote:
> alignment

Done.

https://codereview.chromium.org/595933003/diff/20001/build/android/rezip/Rezi...
build/android/rezip/RezipApk.java:316: System.exit(1);
On 2014/09/30 09:36:33, rmcilroy wrote:
> Are these catches (and the one on 322-323) really necessary?  Won't an
uncaught
> exception have the same effect (stacktrace + non-zero exit code)?

Done.

[Anton, 2014-09-30 14:00:29]

The CQ bit was checked by anton@chromium.org

[commit-bot: I haz the power, 2014-09-30 14:01:32]

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

[commit-bot: I haz the power, 2014-09-30 14:15:09]

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

[commit-bot: I haz the power, 2014-09-30 14:15:10]

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

[Anton, 2014-09-30 16:11:48]

The CQ bit was checked by anton@chromium.org

[commit-bot: I haz the power, 2014-09-30 16:12:47]

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

[commit-bot: I haz the power, 2014-09-30 17:15:49]

Committed patchset #6 (id:100001) as 6206fdfa36b2cc02f12cd54961dc0b84475e92ba

[commit-bot: I haz the power, 2014-09-30 17:16:27]

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