Keep protobuf's parseFrom so the GroupUpdated serialization fix survives the shrinker - #2224
Merged
Merged
Conversation
The serializer rebuilds a protobuf through parseFrom, reached reflectively, so R8 cannot see the call: usage.txt for a minified build listed SessionProtos$GroupUpdateMessage.parseFrom(byte[]) among the removed methods, along with 62 other protobuf parseFrom overloads. The read would have thrown NoSuchMethodException, been caught where the KryoException was caught, and dropped the row exactly as before -- so the fix was inert in every build that ships, and no JVM test could see it because unit tests are not minified. With the rule, none of the parseFrom(byte[]) methods on org.session.protos are removed and the kept seed is listed for the ones we parse. The new test covers the job's own serialize/create pair rather than the shared Kryo alone, so reverting either direction to a bare Kryo fails it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2211, which is already on
dev: the fix it landed does nothing in a build that ships.JobKryo's serializer rebuilds a protobuf throughparseFrom, reached reflectively, so R8 cannot see the call. On a minified build (qainheritsrelease'sisMinifyEnabled), R8's own removal list contained:— along with 62 other protobuf
parseFrom(byte[])overloads.-dontobfuscatekeeps the names, which is what makes this look safe, but shrinking still removes the method. The read would throwNoSuchMethodException, be caught where theKryoExceptionwas caught, and drop the row exactly as before the fix. Unit tests cannot see it because they are not minified.Changes
parseFrom(byte[])on protobuf message classes, next to the existing Kryo section that keepsDestination$*constructors for the same underlying reason.JobKryoTestgains a round trip through the job's ownserialize()/Factory.create()pair rather than the shared Kryo alone — reverting either direction to a bareKryo()now fails it, which the previous test did not catch.Verification
Re-ran
:app:minifyPlayQaWithR8on this branch: noorg.session.protosparseFrom(byte[])appears inusage.txt(63 protobuf ones before the rule, 7 after, all of them DataStore's repackaged protobuf andUnknownFieldSet, none of which we parse through), andSessionProtos$GroupUpdateMessage parseFrom(byte[])is listed inseeds.txtas kept. Full unit suite: 312/312.