IssueTracker

Figured out how to reproduce in a small project: https://github.com/thejk/WriteOnlyFieldR8
The code works in debug and fails in release because R8 removes the write-only mListener field of MyActivity causing the Listener instance to be garbage collected.

If a class has an instance field of a type which has a finalize() method (either directly, in any superclass or in any instantiated sub-class) then that field has to be treated as read by that finalize() method.

However that will be quite conservative, as that will have an implicit read of all instance fields of type Object if there is a single class with finalize() in the program (including libraries used)

class SomeClass {
    private final Object myField;

    SomeClass() {
        if (System.currentTimeMillis() >= 0) {
          myField = new OtherClass();
        } else {
          myField = new Object();
        }
    }
}

It is also possible to do something like this:

class SomeClass {
    private final Object[] objects;

    SomeClass() {
        myField = Objects[]{new OtherClass()};
    }
}

A quick update on this issue: we have a prototype implementation that fixes the issue (such that an explicit keep rule is not necessary) and are currently running some experiments to see if the code size regressions on various benchmarks are sufficiently small to justify landing this as default behavior in R8.

How did it go?

Project: r8
Branch: master

commit 94aaf2134a13303de7a3b5a54cd2c9d060c88fce
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date:   Wed Oct 02 15:26:17 2019

    Treat static-puts that store objects with a non-default finalize() method as having side effects
   
    This should be extended to instance-put instructions as well.
   
    Bug: 137836288
    Change-Id: I8f8afe57da361eeefe4ad5456888511cda90bfdb

M       src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
M       src/main/java/com/android/tools/r8/ir/code/StaticPut.java
A       src/test/java/com/android/tools/r8/ir/analysis/sideeffect/PutObjectWithFinalizeTest.java

https://r8-review.googlesource.com/40849

Project: r8
Branch: master

commit 28a7ade7d8d683e935117abbd70cb2660f7c45ff
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date:   Thu Oct 03 10:28:35 2019

    Reland "Treat static-puts that store objects with a non-default finalize() method as having side effects"
   
    This reverts commit d9c5a2c17319f26cdc08ac79e190eddcbae15cd7.
   
    Bug: 137836288
    Change-Id: I9f03c7cf0ce4410efb208596034274a2a5c33340

M       src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
M       src/main/java/com/android/tools/r8/ir/code/StaticPut.java
A       src/test/java/com/android/tools/r8/ir/analysis/sideeffect/PutObjectWithFinalizeTest.java

https://r8-review.googlesource.com/43902

Project: r8
Branch: master

commit b401eef1e2e17c95e79f02574adecd46a767048c
Author: Christoffer Quist Adamsen <christofferqa@google.com>
Date:   Mon Oct 14 09:50:29 2019

    Treat instance-put as having side-effects if it may store an object with a non-default finalize()
   
    Bug: 137836288
    Change-Id: I939386a925a8eba94eeb27a90d180c1c89a9316f

M       src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
M       src/main/java/com/android/tools/r8/ir/code/FieldInstruction.java
M       src/main/java/com/android/tools/r8/ir/code/InstancePut.java
M       src/main/java/com/android/tools/r8/ir/code/StaticPut.java

https://r8-review.googlesource.com/44118

For the record this is now fixed on ToT. It is possible to build with a version of R8 that has the fix by making the following changes to build.gradle.

    buildscript {
        repositories {
            maven { url "http://storage.googleapis.com/r8-releases/raw/master" }
        }
        dependencies {
            classpath 'com.android.tools:r8:b401eef1e2e17c95e79f02574adecd46a767048c'  // Must be before the Gradle Plugin for Android.
            classpath 'com.android.tools.build:gradle:X.Y.Z'     // Your current AGP version.
         }
    }

Do I understand the fix correctly that it handles only the case when you have a finalize() method? If that's the case then there is more work needed since the finalize() method was only added to this report (and test project) to make it clearly visible that the problem occurs. In our original code we don't have a finalize() method but the field is removed and hence GC:ed, causing the application to break.

Sorry, I am not sure I follow your comment. Could you please clarify why the field cannot be removed even if all values that flow into the field do not have a finalize() method?

Certainly. I think the best way to explain it is to look at the test project attached above. It creates a field with a Listener which gets handed to Manager. Manager only maintains a weak reference to the listener so field in MyActivity is expected to keep the Listener object alive. This does not happen since R8 removes the field, leaving Listener dangling and eventually GC:ed.

Thanks. Weak references can render any optimization that removes any reference invalid if that reference happens to be needed to make sure that something is not cleared at a GC. Unfortunately there is no good way that R8 can detect this. Therefore, your best option is probably to keep these fields explicitly. One way would be to introduce an annotation @ProtectWeakReference and use the keep rule `-keepclassmembers class * { @ProtectWeakReference <fields>; }`, and then annotate MyActivity.mListener with @ProtectWeakReference. With the recent finalize() fixes, another solution would be to provide an empty implementation of finalize() in MyActivity$Listener, since that would prevent R8 from removing the field writes. The advantage of this approach is that you don't need to carefully annotate every field that may hold on to a reference that must not be GC'd.

Ok, that doesn't sound like a very good behavior though since running R8 shouldn't result in broken code, right? Obviously one can trick any optimizer (for example through reflection) to break an app, but the code in question here doesn't feel very exotic so having to manually massage it to please R8 just feels wrong. I don't know the internals of R8 optimizations so I certainly believe you when you say it's hard for R8 to detect but in that case it should be doing the safe thing and preserve the code. There was a comment earlier (#3) suggesting something like:

class SomeClass {
    private final Object[] objects;

    SomeClass() {
        objects = new Object[]{new OtherClass()};
    }
}

Perhaps if a class has multiple write only fields then that could still serve as an optimization without the risk of breaking the code?

Weak references are challenging to handle for the same reasons that reflection is challenging to handle. The challenge with reflection is that there are field reads and writes (among others) that are not visible to the shrinker. In the example app from comment #2 you are relying on the behavior of the GC, but the GC traverses the heap and thus implicitly reads all fields in the reachable part of the heap. R8 does not model that. Of course it would be easy to treat all reference fields as being read by the GC in R8, but that would prevent all reference fields from being removed if they are written, and therefore this is not something we are planning to pursue. When that is said, you can easily model that behavior of the GC by adding the following keep rule to your app:

  # Treat all fields as being read by the GC.
  -keepclassmembers,allowobfuscation class * { <fields>; }

Ok. Thank you for taking the time to motivate why, much appreciated!

Just realized that we're actually stating the following in our proguard configuration:

-optimizations !field/*

This should imply !field/removal/writeonly as far as I understand but R8 does not follow this configuration?

No problem, and thanks for the feedback! Please don't hesitate to file issues in case you run into other R8 related issues. Also, we would be interested to hear about how you end up dealing with weak references in presence of shrinking. If you end up with a rule like `-keepclassmembers,allowobfuscation class * { <fields>; }` to signal that all fields could be read by the GC, then there is room for improvement since that rule also marks all fields as being potentially written. As a side effect of this, even fields that are never written cannot be removed, although such fields are not a problem when it comes to dealing with weak references.

Regarding the -optimizations directive, this is a Proguard-only directive along with a few other directives such as -optimizationpasses. R8 used to output a warning saying that such rules were ignored, but this was downgraded to an info-level message since these warnings were not very actionable for rules coming from third-party Proguard configuration rules. You should see these messages when building with the --info option.

Will mark this as fixed now that R8 no longer removes field assignments that may store an object that has a non-default finalize() implementation. As discussed in comments 10 through 17 there can still be issues in presence of weak references. Unfortunately there is no desirable solution to this problem (see also comment #17), so this requires additional keep rules.

Since you expressed some interest in how we ended up dealing with this: we decided to introduce an annotation similar to @ProtectWeakReference above together with an annotation to mark weakly stored method params, then letting a custom errorprone check look for unsafe usages. So something like:

void myMethod(@Weak Object obj) {
    this.obj = new WeakReference<>(obj);
}

and calling this with for example:

void anotherMethod() {
  Object anObject = new Object();
  myMethod(anObject);
}

would trigger the errorprone check. Instead one would need to do:

@ProtectWeakReference
private final Object myObject = new Object();

void anotherMethod() {
  myMethod(myObject);
}

Hopefully it can serve as some help to anyone else encountering this behavior.

Thanks for taking the time to share this!

Project: r8
Branch: 1.6

commit fb5301320a8fb2c57128e52a347c3464cf9a9375
Author: Søren Gjesse <sgjesse@google.com>
Date:   Tue Nov 05 15:51:47 2019

    Version 1.6.47
   
    Cherry-pick: Synthesize lambda classes prior to each wave
    CL: https://r8-review.googlesource.com/c/r8/+/43986
   
    Cherry-pick: Treat instance-put as having side-effects if it may store an object with a non-default finalize()
    CL: https://r8-review.googlesource.com/c/r8/+/44118
   
    Cherry-pick: Fix may-have-finalize-method cache
    CL: https://r8-review.googlesource.com/c/r8/+/43952
   
    Cherry-pick: Reland "Treat static-puts that store objects with a non-default finalize() method as having side effects"
    CL: https://r8-review.googlesource.com/c/r8/+/43902
   
    Bug: 142203515
    Bug: 137836288
    Change-Id: I06ecc3c1c014619b4bc16545625576a15bda4760

M       src/main/java/com/android/tools/r8/Version.java
M       src/main/java/com/android/tools/r8/graph/AppInfoWithSubtyping.java
M       src/main/java/com/android/tools/r8/ir/code/FieldInstruction.java
M       src/main/java/com/android/tools/r8/ir/code/InstancePut.java
M       src/main/java/com/android/tools/r8/ir/code/StaticPut.java
M       src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
M       src/main/java/com/android/tools/r8/ir/conversion/LensCodeRewriter.java
M       src/main/java/com/android/tools/r8/ir/conversion/MethodProcessor.java
M       src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
A       src/test/java/com/android/tools/r8/ir/analysis/sideeffect/PutObjectWithFinalizeTest.java

https://r8-review.googlesource.com/45187