Use Adjust's recommend proguard config

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 47
All
Android
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed, firefox46 fixed, firefox47 fixed, fennec44+)

Details

Attachments

(1 attachment)

We use some wildcards that could be more efficient (via [1]):

-keep class com.adjust.sdk.plugin.MacAddressUtil { 
    java.lang.String getMacAddress(android.content.Context); 
}
-keep class com.adjust.sdk.plugin.AndroidIdUtil { 
    java.lang.String getAndroidId(android.content.Context); 
}
-keep class com.google.android.gms.common.ConnectionResult { 
    int SUCCESS; 
}
-keep class com.google.android.gms.ads.identifier.AdvertisingIdClient {
    com.google.android.gms.ads.identifier.AdvertisingIdClient$Info 
        getAdvertisingIdInfo (android.content.Context);
}
-keep class com.google.android.gms.ads.identifier.AdvertisingIdClient$Info {
    java.lang.String getId ();
    boolean isLimitAdTrackingEnabled();
}

This could be potentially causing other errors (e.g. bug 1244930).

[1]: https://github.com/adjust/android_sdk#5-add-permissions
Comment on attachment 8715574 [details]
MozReview Request: Bug 1245711 - Update Adjust proguard cfg to match docs. r=mfinkle

https://reviewboard.mozilla.org/r/33515/#review30221

Looks like the Adjust docs updated the Proguard rules here:
https://github.com/adjust/android_sdk/commit/b15279a5d02b5bab4aa1411c8335bc94662e6ac5

Makes sense for us to stay up to date.
Attachment #8715574 - Flags: review?(mark.finkle) → review+
Shall we uplift to Beta?
tracking-fennec: --- → ?
tracking-fennec: ? → 44+
Comment on attachment 8715574 [details]
MozReview Request: Bug 1245711 - Update Adjust proguard cfg to match docs. r=mfinkle

Approval Request Comment
[Feature/regressing bug #]: Original implementation
[User impact if declined]: We are shipping an older version [1] of a Proguard file, so we're not removing as many as the methods as we could. As such, the users are getting a larger download size and less optimized application.

[Describe test coverage new/current, TreeHerder]: Tested locally – no perceivable change.
[Risks and why]: Low – we just copied the latest configuration file from the official adjust docs [2].

[String/UUID change made/needed]: None

[1]: https://github.com/adjust/android_sdk/commit/b15279a5d02b5bab4aa1411c8335bc94662e6ac5#diff-04c6e90faac2675aa89e2176d2eec7d8
[2]: https://github.com/adjust/android_sdk#5-add-permissions
Attachment #8715574 - Flags: approval-mozilla-release?
Attachment #8715574 - Flags: approval-mozilla-beta?
Attachment #8715574 - Flags: approval-mozilla-aurora?

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7c52ceb05b6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Hi Margaret, Mike: Does this patch actually fix the problem in bug 1244930? I am wondering whether we are at a point where we can gtb Fennec 44.0.1 or is there another fix needed for that bug?
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
(In reply to Ritu Kothari (:ritu) from comment #7)
> Hi Margaret, Mike: Does this patch actually fix the problem in bug 1244930?

No. This patch fixes an issue where we used an old revision of Adjust's proguard configuration which was a superset of the latest revision. End result is it should slim down our APK a bit via Proguard optimizations, but it's not necessary. If it was my choice, I'd uplift to Beta.

> I am wondering whether we are at a point where we can gtb Fennec 44.0.1 or
> is there another fix needed for that bug?

The bug for that appears to be bug 1233238.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8715574 [details]
MozReview Request: Bug 1245711 - Update Adjust proguard cfg to match docs. r=mfinkle

Sure, anything to improve adjust. Taking it.
Should be in 45 beta 4.
Attachment #8715574 - Flags: approval-mozilla-beta?
Attachment #8715574 - Flags: approval-mozilla-beta+
Attachment #8715574 - Flags: approval-mozilla-aurora?
Attachment #8715574 - Flags: approval-mozilla-aurora+
Margaret, Mike, Nick: I will gtb build5 for Fennec 44.0.2 shortly. Do we need this patch as well? I know we included the fix from bug 1233238 in 44.0.2. Is this bug fix also needed to ensure adjust works in Fennec? Or is this optional?
Flags: needinfo?(nalexander)
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
(In reply to Ritu Kothari (:ritu) from comment #12)
> Margaret, Mike, Nick: I will gtb build5 for Fennec 44.0.2 shortly. Do we
> need this patch as well? I know we included the fix from bug 1233238 in
> 44.0.2. Is this bug fix also needed to ensure adjust works in Fennec? Or is
> this optional?

To the best of my knowledge, we don't *need* the patch.  It's my belief that our current PG configuration protects *more* than the patch in question, so it should be strictly safer than the patch.  It's possible the current PG configuration is incorrect, but I see no reason to think that.

The advantage of mcomella's patch is that it is *exactly* what upstream Adjust SDK recommends.  So we should unify on it eventually, but it's not a blocker.
Flags: needinfo?(nalexander)
re comment 13, I agree with Nick – see comment 8 for more.

(In reply to Nick Alexander :nalexander from comment #13)
> It's my belief that our current PG configuration protects *more* than the
> patch in question, so it should be strictly safer than the patch.  It's
> possible the current PG configuration is incorrect, but I see no reason to think that.

afaict, that is correct. See this commit [1] from the adjust SDK, with the summary "Narrow down proguard exceptions" that basically does the patch in this bug for their docs.

> The advantage of mcomella's patch is that it is *exactly* what upstream
> Adjust SDK recommends.  So we should unify on it eventually, but it's not a
> blocker.

Right.

[1]: https://github.com/adjust/android_sdk/commit/b15279a5d02b5bab4aa1411c8335bc94662e6ac5#diff-04c6e90faac2675aa89e2176d2eec7d8
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8715574 [details]
MozReview Request: Bug 1245711 - Update Adjust proguard cfg to match docs. r=mfinkle

Thanks Nick and Mike for a quick reply. Based on the fact that this is not a must-have in Fennec 44 (other fixes will get adjust working as expected), I prefer not to take this fix in 44 and let it ride beta45 -> release45 train. Thanks!
Attachment #8715574 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.