Closed Bug 1305461 Opened 3 years ago Closed 3 years ago

Add generic keep annotation to disable ProGuard optimization on fields

Categories

(Firefox for Android :: General, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: esawin, Assigned: esawin)

Details

Attachments

(2 files, 2 obsolete files)

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.
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?
Attachment #8794926 - Flags: review?(michael.l.comella)
Attachment #8794926 - Flags: feedback?(snorp)
Rename the field annotation for the native reference.
Attachment #8794927 - Flags: review?(michael.l.comella)
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
Attachment #8794926 - Flags: feedback?(snorp) → feedback+
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?
(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 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.
Attachment #8794926 - Flags: review?(michael.l.comella) → review?(s.kaspari)
Attachment #8794927 - Flags: review?(michael.l.comella) → review?(s.kaspari)
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
Attachment #8794927 - Flags: review?(s.kaspari)
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 :)
Attachment #8794926 - Flags: review?(s.kaspari)
Attachment #8794926 - Attachment is obsolete: true
Attachment #8796533 - Flags: review?(s.kaspari)
Attachment #8794927 - Attachment is obsolete: true
Attachment #8796535 - Flags: review?(s.kaspari)
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 :)
Attachment #8796533 - Flags: review?(s.kaspari) → review+
Attachment #8796535 - Flags: review?(s.kaspari) → review+
Pushed by esawin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a35bade49303
https://hg.mozilla.org/mozilla-central/rev/5af529f03228
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.