Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Proguard/R8 configuration for createValueMap #668

Merged
merged 2 commits into from
Jun 10, 2020

Conversation

Sloy
Copy link
Contributor

@Sloy Sloy commented Jun 8, 2020

Description of the issue

The method com.segment.analytics.ValueMap#createValueMap uses reflection to create an instance of the given Class<T>. It expects a constructor receiving a Map.

Unfortunately, for some classes like Address, this constructor is not used anywhere else in the code so applications using Proguard or R8 to remove unused code will delete this constructor. The outcome is a runtime crash in the application.

Here's an example extracted from Firebase Crashlytics in our production application:
Screenshot 2020-06-08 at 14 17 51

How to reproduce

We could not reproduce the exact same execution path of our crash, which goes through the AppboyIntegration library. But we can manually force the crash by invoking the getValueMap method:

Traits traits = new Traits();
traits.put("address", new ValueMap());
traits.getValueMap("address", Traits.Address.class);

It can be easily reproduced in the sample module by using the snippet above and enabling R8 in the analytics-samples/build.gradle file:

android {
  // ...
  buildTypes {
    debug {
      minifyEnabled true
    }
  }
}

The proposed fix

We implemented a workaround adding a Proguard rule into our application to stop it from removing code from the Segment SDK.

This PR aims to solve this issue for all consumers of the SDK by providing a consumerProguardFiles. The rule in this file keeps classes containing a constructor receiving a map. Proguard/R8 will keep the name of the class from obfuscation and the constructor from being removed.

I believe this is just enough to avoid similar crashes, but please double-check the solution.
Here's some more info about the setup:

analytics/proguard-rules-lib.txt Outdated Show resolved Hide resolved
Co-authored-by: Prayansh Srivastava <prayansh@users.noreply.github.com>
@Sloy Sloy requested a review from prayansh June 9, 2020 07:55
@egor-n
Copy link
Contributor

egor-n commented Jun 9, 2020

Would it make sense to update the "How should I configure my Proguard?" section now that the library provides built-in rules?

@prayansh
Copy link
Contributor

prayansh commented Jun 9, 2020

@egor-n good call :) . I will make sure those are updated, if any changes are needed.

@prayansh prayansh merged commit 7cebbc8 into segmentio:master Jun 10, 2020
@Sloy Sloy deleted the proguard-rules branch June 10, 2020 06:13
@Sloy Sloy mentioned this pull request Aug 18, 2020
@sahilgarg90
Copy link

@egor-n good call :) . I will make sure those are updated, if any changes are needed.

https://segment.com/docs/connections/sources/catalog/libraries/mobile/android/android-faqs/#how-should-i-configure-proguard

I think this link needs to be update too, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants