PerformanceMetricsState.getForHierarchy API is a bit weird [233421985]

Fixed

Bug

Status Update

No update yet.

Description

py...@gmail.com

created issue #1

May 21, 2022 10:05PM

PerformanceMetricsState.getForHierarchy returns a MetricsStateHolder which is just a wrapper around PerformanceMetricsState?.

As I started using it, I did this:

// Named it "state" because I'm calling "get" on something a class which as a name ending in State.
val state = PerformanceMetricsState.getForHierarchy(view)
state.state?.let { state ->
 state.addState("foo", "bar")
}

So.. I'm getting a state, then getting a state from it or maybe not, and if I do have a state then I can add a state to it!

The other thing that's a bit weird about this is how PerformanceMetricsState is a separate class but it's directly coupled to JankState, i.e. MetricsStateHolder.state will always be set if I already called JankState.createAndTrack() for that window, but it'll be null if I didn't. This conditional nullability isn't super clear.

Suggestions:

Consider hiding MetricsStateHolder and having getForHierarchy directly return PerformanceMetricsState?
Consider a name changes so that we're not adding "state" (addState) to an object already named "state" (PerformanceMetricsState).
Consider either breaking the coupling entirely or making the coupling more explicit. Breaking the coupling would mean deleting the PerformanceMetricsState.create() function that JankStat is calling, and having MetricsStateHolder.getForHierarchy() automatically create it on first call. Making the coupling more explicit might mean making a PerformanceMetricsState instance directly available on a JankState instance.

Comments

py...@gmail.com <py...@gmail.com> #2May 21, 2022 10:15PM

The issue is that Jetifier modifies jars and does not remove signatures. Below is the original class:

Classfile /tmp/non-jetified/org/bouncycastle/jce/provider/ExtCRLException.class
Last modified Mar 1, 2015; size 362 bytes
MD5 checksum 3968a716f9e60e1ad36c34157bd0239f
class org.bouncycastle.jce.provider.ExtCRLException extends java.security.cert.CRLException
minor version: 0
major version: 49
flags: ACC_SUPER
Constant pool:
#1 = Utf8 <init>
#2 = Utf8 cause
#3 = Utf8 getCause
#4 = Utf8 java/security/cert/CRLException
#5 = Utf8 org/bouncycastle/jce/provider/ExtCRLException
#6 = Class #4 // java/security/cert/CRLException
#7 = Class #5 // org/bouncycastle/jce/provider/ExtCRLException
#8 = Utf8 Ljava/lang/Throwable;
#9 = Utf8 (Ljava/lang/String;)V
#10 = Utf8 ()Ljava/lang/Throwable;
#11 = Utf8 (Ljava/lang/String;Ljava/lang/Throwable;)V
#12 = NameAndType #2:#8 // cause:Ljava/lang/Throwable;
#13 = NameAndType #1:#9 // "<init>":(Ljava/lang/String;)V
#14 = Fieldref #7.#12 // org/bouncycastle/jce/provider/ExtCRLException.cause:Ljava/lang/Throwable;
#15 = Methodref #6.#13 // java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
#16 = Utf8 Code
{
java.lang.Throwable cause;
descriptor: Ljava/lang/Throwable;
flags:

org.bouncycastle.jce.provider.ExtCRLException(java.lang.String, java.lang.Throwable);
descriptor: (Ljava/lang/String;Ljava/lang/Throwable;)V
flags:
Code:
stack=2, locals=3, args_size=3
0: aload_0
1: aload_1
2: invokespecial #15 // Method java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
5: aload_0
6: aload_2
7: putfield #14 // Field cause:Ljava/lang/Throwable;
10: return

public java.lang.Throwable getCause();
descriptor: ()Ljava/lang/Throwable;
flags: ACC_PUBLIC
Code:
stack=1, locals=1, args_size=1
0: aload_0
1: getfield #14 // Field cause:Ljava/lang/Throwable;
4: areturn
}

One processed with Jetifier:

Classfile /tmp/jetified/org/bouncycastle/jce/provider/ExtCRLException.class
Last modified Oct 14, 2019; size 362 bytes
MD5 checksum 76bc557706ee3480e53d88b21316913f
class org.bouncycastle.jce.provider.ExtCRLException extends java.security.cert.CRLException
minor version: 0
major version: 49
flags: ACC_SUPER
Constant pool:
#1 = Utf8 org/bouncycastle/jce/provider/ExtCRLException
#2 = Class #1 // org/bouncycastle/jce/provider/ExtCRLException
#3 = Utf8 java/security/cert/CRLException
#4 = Class #3 // java/security/cert/CRLException
#5 = Utf8 cause
#6 = Utf8 Ljava/lang/Throwable;
#7 = Utf8 <init>
#8 = Utf8 (Ljava/lang/String;Ljava/lang/Throwable;)V
#9 = Utf8 (Ljava/lang/String;)V
#10 = NameAndType #7:#9 // "<init>":(Ljava/lang/String;)V
#11 = Methodref #4.#10 // java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
#12 = NameAndType #5:#6 // cause:Ljava/lang/Throwable;
#13 = Fieldref #2.#12 // org/bouncycastle/jce/provider/ExtCRLException.cause:Ljava/lang/Throwable;
#14 = Utf8 getCause
#15 = Utf8 ()Ljava/lang/Throwable;
#16 = Utf8 Code
{
java.lang.Throwable cause;
descriptor: Ljava/lang/Throwable;
flags:

org.bouncycastle.jce.provider.ExtCRLException(java.lang.String, java.lang.Throwable);
descriptor: (Ljava/lang/String;Ljava/lang/Throwable;)V
flags:
Code:
stack=2, locals=3, args_size=3
0: aload_0
1: aload_1
2: invokespecial #11 // Method java/security/cert/CRLException."<init>":(Ljava/lang/String;)V
5: aload_0
6: aload_2
7: putfield #13 // Field cause:Ljava/lang/Throwable;
10: return

public java.lang.Throwable getCause();
descriptor: ()Ljava/lang/Throwable;
flags: ACC_PUBLIC
Code:
stack=1, locals=1, args_size=1
0: aload_0
1: getfield #13 // Field cause:Ljava/lang/Throwable;
4: areturn
}

Changes in the constants pool cause changes in the code (indices change), which causes different MD5 checksums. This means that signatures will not match any more.

This started happening in 3.6.0-beta01 because we are using Jetifier with copyUnmodifiedLibsAlso=true flag (previously it was false).

ch...@google.com <ch...@google.com> #3Jul 11, 2022 10:15PM

@Chris You can work around this by updating gradle.properties with:

android.jetifier.blacklist=.*bcprov.*

This will avoid Jetifying the bouncycastle library.

ch...@google.com <ch...@google.com> #4Jul 11, 2022 10:56PM

Thanks Ivan!

ch...@google.com <ch...@google.com> #5Jul 11, 2022 11:47PM

Pending fix in:

https://android-review.googlesource.com/c/platform/frameworks/support/+/1151460

ap...@google.com <ap...@google.com> #6Jul 13, 2022 08:24PM

did this fix make it in for 3.6?

ch...@google.com <ch...@google.com> Jul 13, 2022 09:21PM

Marked as fixed.

py...@gmail.com <py...@gmail.com> #7Jul 14, 2022 08:44AM

Jetifier 1.0.0-beta08 was released and has a fix for this.

To avoid waiting for new Android Studio you can workaround this in the meantime by forcing usage of a new version of jetifier in gradle:
buildscript {
dependencies {
classpath 'com.android.tools.build:gradle:x.y.z'
classpath 'com.android.tools.build.jetifier:jetifier-processor:1.0.0-beta08' // <- Add this
}
}

ch...@google.com <ch...@google.com> #8Jul 14, 2022 03:59PM

Jetifier 1.0.0-beta08, which has the fix for this issue, will be integrated into AGP 3.6.0-rc01 and 4.0.0-alpha06:

https://android.googlesource.com/platform/tools/base/+/ef5e56702b626b6be642ecce7d0078b7b1a81bcb

Message last modified on Jul 14, 2022 04:00PM