Proposal: DexMaker. generateFileName() should take into account method accessibility

[nfuller]

I just hit a bug which I believe to be the result of changing the accessibility of method on a class from protected to public. I had a spy() on an instance of the class.

DexMaker.generateFileName() generates a hash using (among other things) MethodId. MethodId doesn't include method accessibility (public vs protected) so the cached key of generated code was the same after I made the change.

Therefore, the dexcache continued to pick out the dex generated back when the method was protected.

This had knock-ons elsewhere for me. (For some reason) we have a local patch in Android in ProxyBuilder.getMethodsToProxyRecursive(). This change sorts the methods by Method.toString() (which does include the access modifier). It means the order of $__methodsArray changed.

The upshot was that the cached generated code was indexing into $__methodsArray with different indexes from the ones used at runtime.

This local patch was the immediate cause of my issue, but I propose that generateFileName() should be a bit more conservative when generating the cache key. For example, any change to method signatures that affects the ordering of ProxyBuilder.getMethodsToProxyRecursive() but does not result in a different hash in generateFileName() could cause similar issues.

Even if the ordering of ProxyBuilder.getMethodsToProxyRecursive() were stable in the face of access modifier changes, the generated code pulled out of the cache would have had incorrect access modifiers so it is probably incorrect to treat this as a cache hit.

[drewhannay]

@moltmann Did PR 123 fix this?

[moltmann]

I think PR 123 goes the other way round. It unifies the behavior to ignore accessibility. Maybe we should to unify the behavior to consider accessibility.