Plug leak in class initialization.
Don't add static fields and methods to the resolved-items table if the
defining class is still being initialized. If we do, other threads can
then access those fields and methods without first performing an "is the
class initialized" test.
We will end up performing static field and method resolution more often
now than before. On a Nexus One, zygote class preload time went from
2227ms to 2239ms (i.e. the performance difference is in the noise).
Bug 2655384.
(Re-applying this after the JIT fix.)
Change-Id: I14846865313730f705b819c5893456e342316e2e
diff --git a/vm/oo/Class.c b/vm/oo/Class.c
index b979ff9..05eccdc 100644
--- a/vm/oo/Class.c
+++ b/vm/oo/Class.c
@@ -4208,6 +4208,11 @@
*
* We will often be called recursively, e.g. when the <clinit> code resolves
* one of its fields, the field resolution will try to initialize the class.
+ * In that case we will return "true" even though the class isn't actually
+ * ready to go. The ambiguity can be resolved with dvmIsClassInitializing().
+ * (TODO: consider having this return an enum to avoid the extra call --
+ * return -1 on failure, 0 on success, 1 on still-initializing. Looks like
+ * dvmIsClassInitializing() is always paired with *Initialized())
*
* This can get very interesting if a class has a static field initialized
* to a new instance of itself. <clinit> will end up calling <init> on
diff --git a/vm/oo/Resolve.c b/vm/oo/Resolve.c
index 68fdd51..513a282 100644
--- a/vm/oo/Resolve.c
+++ b/vm/oo/Resolve.c
@@ -13,6 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
/*
* Resolve classes, methods, fields, and strings.
*
@@ -256,10 +257,21 @@
}
/*
- * The class is initialized, the method has been found. Add a pointer
- * to our data structure so we don't have to jump through the hoops again.
+ * If the class has been initialized, add a pointer to our data structure
+ * so we don't have to jump through the hoops again. If this is a
+ * static method and the defining class is still initializing (i.e. this
+ * thread is executing <clinit>), don't do the store, otherwise other
+ * threads could call the method without waiting for class init to finish.
*/
- dvmDexSetResolvedMethod(pDvmDex, methodIdx, resMethod);
+ if (methodType == METHOD_STATIC && !dvmIsClassInitialized(resMethod->clazz))
+ {
+ LOGVV("--- not caching resolved method %s.%s (class init=%d/%d)\n",
+ resMethod->clazz->descriptor, resMethod->name,
+ dvmIsClassInitializing(resMethod->clazz),
+ dvmIsClassInitialized(resMethod->clazz));
+ } else {
+ dvmDexSetResolvedMethod(pDvmDex, methodIdx, resMethod);
+ }
return resMethod;
}
@@ -366,8 +378,11 @@
//assert(dvmIsClassInitialized(resMethod->clazz));
/*
- * The class is initialized, the method has been found. Add a pointer
- * to our data structure so we don't have to jump through the hoops again.
+ * Add a pointer to our data structure so we don't have to jump
+ * through the hoops again.
+ *
+ * As noted above, no need to worry about whether the interface that
+ * defines the method has been or is currently executing <clinit>.
*/
dvmDexSetResolvedMethod(pDvmDex, methodIdx, resMethod);
@@ -419,8 +434,14 @@
dvmIsClassInitializing(resField->field.clazz));
/*
- * The class is initialized, the method has been found. Add a pointer
- * to our data structure so we don't have to jump through the hoops again.
+ * The class is initialized (or initializing), the field has been
+ * found. Add a pointer to our data structure so we don't have to
+ * jump through the hoops again.
+ *
+ * Anything that uses the resolved table entry must have an instance
+ * of the class, so any class init activity has already happened (or
+ * been deliberately bypassed when <clinit> created an instance).
+ * So it's always okay to update the table.
*/
dvmDexSetResolvedField(pDvmDex, ifieldIdx, (Field*)resField);
LOGVV(" field %u is %s.%s\n",
@@ -476,10 +497,20 @@
}
/*
- * The class is initialized, the method has been found. Add a pointer
- * to our data structure so we don't have to jump through the hoops again.
+ * If the class has been initialized, add a pointer to our data structure
+ * so we don't have to jump through the hoops again. If it's still
+ * initializing (i.e. this thread is executing <clinit>), don't do
+ * the store, otherwise other threads could use the field without waiting
+ * for class init to finish.
*/
- dvmDexSetResolvedField(pDvmDex, sfieldIdx, (Field*) resField);
+ if (dvmIsClassInitialized(resField->field.clazz)) {
+ dvmDexSetResolvedField(pDvmDex, sfieldIdx, (Field*) resField);
+ } else {
+ LOGVV("--- not caching resolved field %s.%s (class init=%d/%d)\n",
+ resField->field.clazz->descriptor, resField->field.name,
+ dvmIsClassInitializing(resField->field.clazz),
+ dvmIsClassInitialized(resField->field.clazz));
+ }
return resField;
}
