ProGuard config ignored in two passes

[arnaudvenet]

The optimization passes ReBindRefs and Unterface do not use the can_* functions of ReachableClasses and could potentially modify classes that are protected in the ProGuard configuration.

[satnam6502]

Indeed. Right now we do not choose to be bound by the incoming ProGuard rules for each and every optimization because often ProGuard rules are overly broad / coarse and prevent us from applying otherwise worthwhile and valid transformations. The entire ProGuard processing pipeline is about to be overhauled.

[wisechengyi]

Seems related. I can create a separate ticket if not.

The default proguard-android.txt has

# Keep setters in Views so that animations can still work.
-keepclassmembers public class * extends android.view.View {
    void set*(***);
    *** get*();
}

which works fine for some private set/get methods in a class extending android.view.View in a proguarded apk.

private void setTextXOffset(float offset) {...}
private void setPillColor(@ColorInt int color) {...}

but after redexing, it has the following error:

06-08 17:12:11.910 13238 13238 W PropertyValuesHolder: Method setPillColor() with type int not found on target class class com.twitter.ui.widget.PillToggleButton
06-08 17:12:11.911 13238 13238 W PropertyValuesHolder: Method setTextXOffset() with type float not found on target class class com.twitter.ui.widget.PillToggleButton

Making the get/set methods public works around the issue for the moment.

Sha is e496a998bf37647b01387641fd7cef4dbf97bd65

Edit: another aspect is that I think it is fine to further optimize a private method, but in this case, it seems like it optimized the private method, but did not change wherever it is being called in the class.

[justinjhendrick]

This sounds like a bug in SimpleInlinePass. @dariorussi, I notice you've touched that file the most, would you like to take a look?

[justinjhendrick]

@wisechengyi, could you give us the config (both proguard and redex) you're using? And (if possible) the apk too. It'll help us debug.

[wisechengyi]

Thanks for the prompt reply @justinjhendrick.

I cannot share the apk directly, but I will try to create a dummy app that repro the issue.

[wisechengyi]

Unfortunately I cannot repro it with a dummy app. I will try to see if I can repro this with a publicly available version + the proguard configs.

[wisechengyi]

@justinjhendrick @dariorussi Please see the info as below:

Repro step:

1. Go through the sign up step (don't mean to trick you to be a twitter user, but there's no better way since we cannot provide the debug builds...) until you hit this step
2. Click on any of the pills. the background should become blue (with release.apk), but with redexed apk it will the error (I copied the error message from the above, as with release builds it is hard to tell from the log):

06-08 17:12:11.910 13238 13238 W PropertyValuesHolder: Method setPillColor() with type int not found on target class class com.twitter.ui.widget.PillToggleButton
06-08 17:12:11.911 13238 13238 W PropertyValuesHolder: Method setTextXOffset() with type float not found on target class class com.twitter.ui.widget.PillToggleButton

and the background will white out.

release.apk: https://www.dropbox.com/s/iyxohg7knqmyn0y/release.apk?dl=0 release-redexed.apk: https://www.dropbox.com/s/vvhcyl8e0jrdy3i/release-redexed.apk?dl=0 proguard configs: archive.zip

redex sha is e496a998bf37647b01387641fd7cef4dbf97bd65, it has to be on this sha otherwise it will hit #192, which we can work on next.

redex release.apk -o  release-redexed.apk \
--proguard-config  ... \
--proguard-config  ... \
...(all proguard configs in the zip)
--sign  --keystore ... --keyalias ...  --keypass  ...

[wisechengyi]

setPillColor and setTextXOffset are preserved in release.apk, but partially missing in release-redexed.apk

By unzipping the apks:

$ grep setPillColor -irn release/
Binary file release/classes3.dex matches
Binary file release/classes4.dex matches

$ grep setPillColor -irn release_redexed/
Binary file release_redexed/classes4.dex matches

[justinjhendrick]

Sorry for the delay, I was out on PTO for the last week. I'll start investigating this today

[justinjhendrick]

Here's what I've seen so far:

• setPillColor is only called from one place
• redex inlined it (in that one place) and deleted the method

So, that seems fine. Are you using reflection to reference this method somehow?

Also, could you give me your redex config? I don't see it in the archive you sent.

[minjang]

Thanks @justinjhendrick for following up! And glad to hear that @wisechengyi uses our Redex seriously for the Twitter. I didn't look at the APK but have a few thoughts:

1. Don't grep binaries. I would use dexdump -d a.dex > a.dump and then grep.
2. If you enabled RenameClassesPassV2, the class name would have been changed.
3. Double check this bug reproduces even after you add -dontobfuscate in the ProGuard config.
4. If you think SimpleInlinePass would be a culprit, we have a black list.

"SimpleInlinePass": {
  "black_list": [
    "Lcom/twitter/foo/class1;",
    "Lcom/twitter/foo/class2;"
  ],
  "caller_black_list": [
    "Lcom/twitter/foo/class1;",
    "Lcom/twitter/foo/class2;"
  ],
}

[justinjhendrick]

Thanks @minjang, that's useful advice. @wisechengyi, if you don't want to add every set/get method in an android.View to the redex SimpleInlinePass blacklist, I think you could also add -dontshrink to your proguard config for these methods.

Here's why I think that'll work:

bool can_delete() const {
  return !m_bytype && (!m_keep || m_allowshrinking);
}

from https://github.com/facebook/redex/blob/master/libredex/ReferencedState.h#L50

[wisechengyi]

Thanks for timely investigation, @justinjhendrick and @minjang! It does look like the animation scheme is using some kind of reflection, so we will try some of the options listed in the config. Speaking of which, is there any doc/examples on the redex config so I can have a good idea what can be configured?

Currently we do not have a redex config (so just whatever comes with vanilla).

In the meantime, it will be great if you could take a look at #192, which can be reproduced with the set of apks and proguard configs above.

[justinjhendrick]

config/default.config is quite outdated. I'll update it soon.

But, generally you can do a little research on the passes and configurations available to you by looking at opt/*/*Pass.h.

[justinjhendrick]

One quick amendment, I think the SimpleInline blacklists take class names, not method names

"SimpleInlinePass": {
  "black_list": [
    "Lcom/twitter/foo/ClassBar;",
    "Lcom/twitter/foo/ClassBaz;"
  ],
  "caller_black_list": [
    "Lcom/twitter/foo/ClassFoo;",
    "Lcom/twitter/foo/ClassBaz;"
  ],
}

[minjang]

@justinjhendrick is right. The blacklists are the class names. Sorry for the confusion.

[justinjhendrick]

Another useful thing, you can annotate any class, method, field, etc. with @DoNotStrip, which redex does obey (unlike the PG configs)

and add this to your redex config (at the uppermost level of the json) to prevent deletion:

"keep_annotations": [
  "Lcom/path/to/your/DoNotStrip;"
]

and add this to your config to prevent renaming:

"RenameClassesPassV2" : {
  "###": "don't rename classes that have these annotations",
  "dont_rename_annotated": [
    "Lcom/path/to/your/DoNotStrip;"
  ]
}

and define DoNotStrip

package com.path.to.your;
public @interface DoNotStrip {}

[wisechengyi]

oh wow that's definitely a better approach! otherwise there would be a lot of effort to keep track of the redex config and the source code changes. thanks so much!

[justinjhendrick]

Closing because we found a solution. If @DoNotStrip fails, please re-open.

[wisechengyi]

{
  "redex": {
    "passes" : [
      "ReBindRefsPass",
      "BridgePass",
      "SynthPass",
      "FinalInlinePass",
      "DelSuperPass",
      "SingleImplPass",
      "SimpleInlinePass",
      "StaticReloPass",
      "RemoveEmptyClassesPass",
      "ShortenSrcStringsPass",
      "RenameClassesPassV2"
    ]
  },
  "SimpleInlinePass": {
    "callee_invoke_direct": true,
    "super_same_class": true,
    "virtual_same_class": true,
    "use_liveness": true,
    "throws": true,
    "multiple_callers": true
  },
  "RenameClassesPassV2": {
    "###": "don't rename classes that have these annotations",
    "dont_rename_annotated": [
      "Lcom/twitter/util/annotation/RedexDoNotStrip;"
    ]
  },
  "keep_annotations": [
    "Lcom/twitter/util/annotation/RedexDoNotStrip;"
  ]
}

On [e496a998bf37647b01387641fd7cef4dbf97bd65], it get the following error for the above config.

No -tsa or -tsacert is provided and this jar is not timestamped. Without a timestamp, users may not be able to validate this jar after the signer certificate's expiration date (2047-03-21) or after any future revocation date.
Couldn't find zipalign. See README.md to resolve this.
Traceback (most recent call last):
  File "/tmp/redex.WaKc2r/redex.py", line 505, in <module>
    run_redex(args)
  File "/tmp/redex.WaKc2r/redex.py", line 489, in run_redex
    'RenameClassesPassV2' not in passes_list
AssertionError

RenameClassesPassV2 is obviously in the config, and based on the source code, it should be in the passes list as well, so I'm a bit confused there.

[wisechengyi]

Does does https://github.com/facebook/redex/blob/687f8db9cdc352061d95de2824558741eeb2572c/redex.py#L551-L560 imply that proguard mapping is required in order to use RenameClassesPassV2?

(also I cannot reopen this issue)

[justinjhendrick]

It seems it is required... I'll look into it.

[justinjhendrick]

How are you invoking redex? It's kinda confusing, but default.config isn't used if you don't specify a config (with -c foo.config) :( https://github.com/facebook/redex/blob/master/tools/redex-all/main.cpp#L141

I'll hopefully fix that soon (but I'm not sure if we can depend on redex-all finding the config directory reliably)

[justinjhendrick]

Okay, looking at it some more... It failed that assertion, so 'RenameClassesPassV2' is in the pass list (and it's not allowed to be without the proguard map). Are you using proguard before redex? (like we do) If so, I'll figure out how we pipe in the proguard map and help you do the same.

[justinjhendrick]

The proguard mapping file should be at <project root>/<module name>/build/outputs/mapping/<build type>/<appname>-proguard-mapping.txt and you can tell redex about it like this --proguard-map path/to/mapping.txt

But why you're trying to use RenameClassesPassV2? I looked at the dexdump of twitter.apk and all your classes are already renamed to short things.