linkedin / linkedin/dexmaker

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

Open
#70 2 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.