Refactor/add tests for ObjectStreamClass computeDefaultSUID

b/29064453 added a patch to the computeDefaultSUID() method to maintain
backwards compatibility in how it is calculated for apps that target
<= 23. This change clarifies the behavior by refactoring the code and
adding a comment as well as adding appropriate change markers to
ObjectStreamClass.c. It also adds tests to verify the impact of those
changes on the calculation of the serial version UID.

The original code used a variable/parameter called checkSuperclass that
was confusing as it related to how the native method was implemented
rather than the external effect. The native code works as follows:

1) In order to find the <clinit> method the native code uses a method
   that searches up the super class hierarchy. That means that if a
   class does not have a <clinit> method but a superclass does then it
   will find the superclass' method.

2) If it does not find a method then it returns false which is correct
   behavior. If it does find a method then it checks the value of the
   checkSuperclass parameter.

3) If checkSuperclass is false then the native code returns true
   because it assumes the method it found belongs to the initial class.
   That is incorrect behavior (kept for backwards compatibility) and
   confusing because even though checkSuperclass is false it has
   actually checked the super class.

4) If checkSuperclass is true then the native code will run the same
   search as it did in #1 but on the super class. If no method could
   be found or the method was different to the one found in #1 then it
   returns true as the method found in #1 must have come from the
   original class. Otherwise, it returns false as the method found in
   #1 was actually from the super class. This is confusing because it
   actually checks the super class twice.

Hence the checkSuperclass parameter was inverted and renamed to
inheritStaticInitializer as that describes the behavior and seems
clearer.

That broke the harmony test ObjectStreamClassTest#testHasClinit(). That
test was very low level that does not test any externally visible
behavior but calls the hasStaticInitializer(...) method directly.
Rather than fix that the test was removed and replaced with tests that
check the behavior of that class by its external impact. The test is
described below.

The supported way to access the SUID for a class is to use:
    ObjectStreamClass.lookup(Class).getSerialVersionUID()

If the Class supplied to the lookup() method does not have a
serialVersionUID field then that will call the
computeDefaultSUID(Class) method to compute the default SUID.
Unfortunately, the lookup(Class) method caches the results of the class
lookup (including the computed default SUID) which makes it impossible
to test the behavior of the method on the same Class for version <= 23
and for version > 23 in the same application. Each test has to have its
own Class, with its own default SUID making it difficult to understand
which tests are affected by the version and which are not.

So, instead of using the lookup(Class) method the tests use reflection
to invoke the computeDefaultSUID(Class) method directly, bypassing the
lookup cache and allowing the default SUID to be computed multiple
times for the same class. Each Class has two tests, one for testing
version <= 23 and one for version > 23. For those Classes whose default
SUID is unaffected by the version the tests use a common constant for
the expected SUID.

Tested by running the tests on the device before and after cleaning up
the code.

Test: atest \
    core-tests:libcore.java.io.ObjectStreamClassTest \
    org.apache.harmony.tests.java.io.ObjectStreamClassTest

Bug: 109930347
Change-Id: I245118af8a81ffba123d773e37560ef655e108a3

4 files changed, 166 insertions, 70 deletions

diff --git a/harmony-tests/src/test/java/org/apache/harmony/tests/java/io/ObjectStreamClassTest.java b/harmony-tests/src/test/java/org/apache/harmony/tests/java/io/ObjectStreamClassTest.java
index cc397078ef..16a7415090 100644
--- a/harmony-tests/src/test/java/org/apache/harmony/tests/java/io/ObjectStreamClassTest.java
+++ b/harmony-tests/src/test/java/org/apache/harmony/tests/java/io/ObjectStreamClassTest.java
@@ -284,64 +284,6 @@ public class ObjectStreamClassTest extends TestCaseWithRules {
         return newInstance;
     }
 
-    // Class without <clinit> method
-    public static class NoClinitParent {
-    }
-    // Class without <clinit> method
-    public static class NoClinitChildWithNoClinitParent extends NoClinitParent {
-    }
-
-    // Class with <clinit> method
-    public static class ClinitParent {
-        // This field will trigger creation of <clinit> method for this class
-        private static final String TAG = ClinitParent.class.getName();
-        static {
-
-        }
-    }
-    // Class without <clinit> but with parent that has <clinit> method
-    public static class NoClinitChildWithClinitParent extends ClinitParent {
-    }
-
-    // http://b/29064453
-    public void testHasClinit() throws Exception {
-        Method hasStaticInitializer =
-            ObjectStreamClass.class.getDeclaredMethod("hasStaticInitializer", Class.class,
-                                                      boolean.class);
-        hasStaticInitializer.setAccessible(true);
-
-        assertTrue((Boolean)
-                   hasStaticInitializer.invoke(null, ClinitParent.class,
-                                               false /* checkSuperclass */));
-
-        // RI will return correctly False in this case, but android has been returning true
-        // in this particular case. We're returning true to enable deserializing classes
-        // like NoClinitChildWithClinitParent without explicit serialVersionID field.
-        assertTrue((Boolean)
-                   hasStaticInitializer.invoke(null, NoClinitChildWithClinitParent.class,
-                                               false /* checkSuperclass */));
-        assertFalse((Boolean)
-                    hasStaticInitializer.invoke(null, NoClinitParent.class,
-                                                false /* checkSuperclass */));
-        assertFalse((Boolean)
-                    hasStaticInitializer.invoke(null, NoClinitChildWithNoClinitParent.class,
-                                                false /* checkSuperclass */));
-
-
-        assertTrue((Boolean)
-                   hasStaticInitializer.invoke(null, ClinitParent.class,
-                                               true /* checkSuperclass */));
-        assertFalse((Boolean)
-                   hasStaticInitializer.invoke(null, NoClinitChildWithClinitParent.class,
-                                               true /* checkSuperclass */));
-        assertFalse((Boolean)
-                    hasStaticInitializer.invoke(null, NoClinitParent.class,
-                                                true /* checkSuperclass */));
-        assertFalse((Boolean)
-                    hasStaticInitializer.invoke(null, NoClinitChildWithNoClinitParent.class,
-                                                true /* checkSuperclass */));
-    }
-
     // http://b/29721023
     public void testClassWithSameFieldName() throws Exception {
         // Load class from dex, it's not possible to create a class with same-named
diff --git a/luni/src/test/java/libcore/java/io/ObjectStreamClassTest.java b/luni/src/test/java/libcore/java/io/ObjectStreamClassTest.java
new file mode 100644
index 0000000000..612b7d8798
--- /dev/null
+++ b/luni/src/test/java/libcore/java/io/ObjectStreamClassTest.java
@@ -0,0 +1,135 @@
+/*
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package libcore.java.io;
+
+import java.io.ObjectStreamClass;
+import java.io.Serializable;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import junitparams.JUnitParamsRunner;
+import junitparams.Parameters;
+import libcore.junit.util.SwitchTargetSdkVersionRule;
+import libcore.junit.util.SwitchTargetSdkVersionRule.TargetSdkVersion;
+import org.junit.FixMethodOrder;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestRule;
+import org.junit.runner.RunWith;
+import org.junit.runners.MethodSorters;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(JUnitParamsRunner.class)
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
+public class ObjectStreamClassTest {
+
+  @Rule
+  public TestRule switchTargetSdkVersionRule = SwitchTargetSdkVersionRule.getInstance();
+
+  /**
+   * The default SUID for this should not be affected by the b/29064453 patch.
+   */
+  private static class BaseWithStaticInitializer implements Serializable {
+    static {
+      System.out.println(
+          "Static initializer for " + BaseWithoutStaticInitializer.class.getCanonicalName());
+    }
+  }
+
+  /**
+   * The default SUID for this should not be affected by the b/29064453 patch.
+   */
+  private static class BaseWithoutStaticInitializer implements Serializable {
+  }
+
+  /**
+   * The default SUID for this should not be affected by the b/29064453 patch.
+   */
+  private static class WithStaticInitializer extends BaseWithoutStaticInitializer {
+    static {
+      System.out.println(
+          "Static initializer for " + WithStaticInitializer.class.getCanonicalName());
+    }
+  }
+
+  /**
+   * The default SUID for this should not be affected by the b/29064453 patch.
+   */
+  private static class WithoutStaticInitializer extends BaseWithoutStaticInitializer {
+  }
+
+  /**
+   * The default SUID for this should be affected by the b/29064453 patch and so should differ
+   * between version <= 23 and version > 23.
+   */
+  private static class InheritStaticInitializer extends BaseWithStaticInitializer {
+  }
+
+  public static Object[][] defaultSUIDs() {
+    return new Object[][] {
+        // The default SUID for BaseWithStaticInitializer should not be affected by the b/29064453
+        // patch.
+        { BaseWithStaticInitializer.class, -4971959491244110788L, -4971959491244110788L },
+
+        // The default SUID for BaseWithoutStaticInitializer should not be affected by the
+        // b/29064453 patch.
+        { BaseWithoutStaticInitializer.class, -245652310465925293L, -245652310465925293L },
+
+        // The default SUID for WithStaticInitializer should not be affected by the b/29064453
+        // patch.
+        { WithStaticInitializer.class, -3581047773254885060L, -3581047773254885060L },
+
+        // The default SUID for WithStaticInitializer should not be affected by the
+        // b/29064453 patch.
+        { WithoutStaticInitializer.class, -975889567651927545L, -975889567651927545L },
+
+        // The default SUID for the InheritStaticInitializer should be affected by the b/29064453
+        // patch and so should differ between version <= 23 and version > 23.
+        { InheritStaticInitializer.class, 4188245044387716731L, 992629205079295334L },
+
+
+    };
+  }
+
+  @Parameters(method = "defaultSUIDs")
+  @Test
+  public void computeDefaultSUID_current(Class<?> clazz, long suid,
+      @SuppressWarnings("unused") long suid23) {
+    checkSerialVersionUID(suid, clazz);
+  }
+
+  @Parameters(method = "defaultSUIDs")
+  @Test
+  @TargetSdkVersion(23)
+  public void computeDefaultSUID_targetSdkVersion_23(Class<?> clazz,
+      @SuppressWarnings("unused") long suid, long suid23) {
+    checkSerialVersionUID(suid23, clazz);
+  }
+
+  private static void checkSerialVersionUID(long expectedSUID, Class<?> clazz) {
+    // Use reflection to access the private static computeDefaultSUID method.
+    long defaultSUID;
+    try {
+      Method computeDefaultSUIDMethod =
+          ObjectStreamClass.class.getDeclaredMethod("computeDefaultSUID", Class.class);
+      computeDefaultSUIDMethod.setAccessible(true);
+      defaultSUID = (Long) computeDefaultSUIDMethod.invoke(null, clazz);
+    } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) {
+      throw new IllegalStateException(e);
+    }
+    assertEquals(expectedSUID, defaultSUID);
+  }
+}
diff --git a/ojluni/src/main/java/java/io/ObjectStreamClass.java b/ojluni/src/main/java/java/io/ObjectStreamClass.java
index 4a183f807a..7561aa89da 100644
--- a/ojluni/src/main/java/java/io/ObjectStreamClass.java
+++ b/ojluni/src/main/java/java/io/ObjectStreamClass.java
@@ -1798,9 +1798,14 @@ public class ObjectStreamClass implements Serializable {
             }
 
             // Android-changed: Clinit serialization workaround b/29064453
-            boolean checkSuperclass = !(VMRuntime.getRuntime().getTargetSdkVersion()
-                                       <= MAX_SDK_TARGET_FOR_CLINIT_UIDGEN_WORKAROUND);
-            if (hasStaticInitializer(cl, checkSuperclass)) {
+            // Prior to SDK 24 hasStaticInitializer() would return true if the superclass had a
+            // static initializer, that was contrary to the specification. In SDK 24 the default
+            // behavior was corrected but the old behavior was preserved for apps that targeted 23
+            // or below in order to maintain backwards compatibility.
+            boolean inheritStaticInitializer =
+                (VMRuntime.getRuntime().getTargetSdkVersion()
+                <= MAX_SDK_TARGET_FOR_CLINIT_UIDGEN_WORKAROUND);
+            if (hasStaticInitializer(cl, inheritStaticInitializer)) {
                 dout.writeUTF("<clinit>");
                 dout.writeInt(Modifier.STATIC);
                 dout.writeUTF("()V");
@@ -1880,10 +1885,14 @@ public class ObjectStreamClass implements Serializable {
     /**
      * Returns true if the given class defines a static initializer method,
      * false otherwise.
-     * if checkSuperclass is false, we use a buggy version (for compatibility reason) that
-     * will return true even if only the superclass has a static initializer method.
+     *
+     * @param inheritStaticInitializer if false then this method will return true iff the given
+     * class has its own static initializer, if true (used for backwards compatibility for apps
+     * that target SDK version <= {@link #MAX_SDK_TARGET_FOR_CLINIT_UIDGEN_WORKAROUND}) it will
+     * return true if the given class or any of its ancestor classes have a static initializer.
      */
-    private native static boolean hasStaticInitializer(Class<?> cl, boolean checkSuperclass);
+    private native static boolean hasStaticInitializer(
+        Class<?> cl, boolean inheritStaticInitializer);
     // END Android-changed: Clinit serialization workaround b/29064453
 
     /**
diff --git a/ojluni/src/main/native/ObjectStreamClass.c b/ojluni/src/main/native/ObjectStreamClass.c
index 95cd4f0247..3d3fdefa33 100644
--- a/ojluni/src/main/native/ObjectStreamClass.c
+++ b/ojluni/src/main/native/ObjectStreamClass.c
@@ -51,12 +51,19 @@ static void ObjectStreamClass_initNative(JNIEnv *env)
  * otherwise.
  */
 JNIEXPORT jboolean JNICALL
+// Android-changed: Added inheritStaticInitializer parameter.
+// The inheritStaticInitializer parameter is set to JNI_TRUE when behavior compatible with
+// Android version 23 is required.
 ObjectStreamClass_hasStaticInitializer(JNIEnv *env, jclass this,
                                        jclass clazz,
-                                       jboolean checkSuperclass)
+                                       jboolean inheritStaticInitializer)
 {
     jclass superCl = NULL;
     jmethodID superClinitId = NULL;
+
+    // Android-changed: Added comment to explain behavior.
+    // Search for a static initializer method in this class and up through its
+    // ancestor super classes, returning the id of the first method found.
     jmethodID clinitId =
         (*env)->GetStaticMethodID(env, clazz, "<clinit>", "()V");
     if (clinitId == NULL) {     /* error thrown */
@@ -68,13 +75,15 @@ ObjectStreamClass_hasStaticInitializer(JNIEnv *env, jclass this,
         return JNI_FALSE;
     }
 
-    // Android-changed, if checkSuperclass == true, remove check for
-    // superclass clinitId != child clinitId.
-    // We're returning true to enable deserializing classes without explicit serialVersionID
-    // that would fail in this check (b/29064453).
-    if (checkSuperclass == JNI_FALSE) {
+    // BEGIN Android-changed: Exit immediately if version 23 behavior is required.
+    // At this point the class or one of its ancestor super classes has a
+    // static initializer. If inheritStaticInitializer is true then this method
+    // can simply return true. Otherwise, it needs to check that the static
+    // initializer was not inherited (b/29064453).
+    if (inheritStaticInitializer == JNI_TRUE) {
         return JNI_TRUE;
     }
+    // END Android-changed: Exit immediately if version 23 behavior is required.
 
     /*
      * Check superclass for static initializer as well--if the same method ID
@@ -83,6 +92,7 @@ ObjectStreamClass_hasStaticInitializer(JNIEnv *env, jclass this,
      * JNI spec makes no guarantee that GetStaticMethodID will not return the
      * ID for a superclass initializer.
      */
+
     if ((superCl = (*env)->GetSuperclass(env, clazz)) == NULL) {
         return JNI_TRUE;
     }