Proposal: DexMaker. generateFileName() should take into account method accessibility
- Dominant language
- Java
- Stars
- 2k
- Forks
- 262
- PR merge metrics
- No merged PRs in 30d
Description
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.
Contributor guide
Research direction
Start with DexMaker.generateFileName() and MethodId to trace which method properties enter the cache key, then inspect ProxyBuilder.getMethodsToProxyRecursive() and the $__methodsArray ordering described in the report. Done means generated code is not reused when accessibility or another ordering-relevant signature change would make the cached code incorrect; add or update coverage if the existing test structure supports it.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- android, java
- Domain
- devtools, mobile-dev
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100