1305461 - Add generic keep annotation to disable ProGuard optimization on fields

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Eugen Sawin [:esawin]]

We want to have a more generic "keep this" annotation to prevent ProGuard from optimizing fields out which are required, e.g., for lifecycle management of native objects. See https://bugzilla.mozilla.org/show_bug.cgi?id=1302189#c18.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1305461-1.1-Add-Keep-annotation-to-prevent-ProGu.patch

Adds generic @Keep annotation to prevent removal of required fields for native lifecycle management.

We could make the annotation name more verbose, but @Keep seemed expressive enough and in-line with the ProGuard language to me.

Any better ideas, mcomella or snorp?

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1305461-2.1-Replace-JNITarget-annotation-with-Ke.patch

Rename the field annotation for the native reference.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8794926 [details] [diff] [review]
0001-Bug-1305461-1.1-Add-Keep-annotation-to-prevent-ProGu.patch

Review of attachment 8794926 [details] [diff] [review]:
-----------------------------------------------------------------

I like it

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

There's a @Keep annotation in the list of support annotations:
https://developer.android.com/reference/android/support/annotation/Keep.html

Maybe we can use that?

Comment 5

[Eugen Sawin [:esawin]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> There's a @Keep annotation in the list of support annotations:
> https://developer.android.com/reference/android/support/annotation/Keep.html
> 
> Maybe we can use that?

Looks like we can avoid adding the new interface with this, I can update the patch to use that instead. We'll still need the ProGuard config update.

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8794926 [details] [diff] [review]
0001-Bug-1305461-1.1-Add-Keep-annotation-to-prevent-ProGu.patch

Apparently, we don't have android.* in mozglue.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

I've been talking to nalexander on IRC and there might be an easy way to use the support annotations. We'd need to do two things:

1) Add the support annotations JAR to mozglue
 * Like we added the JAR here [1], we need to add it here [2] too.

2) Add the proguard configuration from the Android SDK (It includes the rules for @Keep)
 * Copy the current version from SDK/tools/proguard/proguard-android.txt to mobile/android/config/proguard/
 * Add an include line to our proguard config in [3]

Do you want to try that?

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#38
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#106
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/config/proguard/proguard.cfg#269

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8794926 [details] [diff] [review]
0001-Bug-1305461-1.1-Add-Keep-annotation-to-prevent-ProGu.patch

I'll clear the review. Just reflag me when needed :)

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1305461-1.2-Add-Android-support-annotation-JAR-t.patch

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1305461-2.2-Replace-JNITarget-annotation-with-Ke.patch

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8796533 [details] [diff] [review]
0001-Bug-1305461-1.2-Add-Android-support-annotation-JAR-t.patch

Review of attachment 8796533 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome :)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a35bade49303
[1.2] Add Android support annotation JAR to mozglue and import Android ProGuard config for extended annotation support. r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/5af529f03228
[2.2] Replace @JNITarget annotation with @Keep for native reference field. r=sebastian

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a35bade49303
https://hg.mozilla.org/mozilla-central/rev/5af529f03228