Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

minifyEnabled stops working at API 0.9.53 onwards: Location has no zero argument constructor #55

Closed
Falcosc opened this issue Jan 20, 2025 · 15 comments
Assignees
Labels

Comments

@Falcosc
Copy link

Falcosc commented Jan 20, 2025

Update Container is requested like this:
https://github.com/Falcosc/locus-addon-tasker/blob/9a2b3c8607d7d07bb8a9bb6fd4c0fb14c26e2692/app/src/main/java/falcosc/locus/addon/tasker/utils/LocusCache.java#L188

And I get the values via the provided field getters
https://github.com/Falcosc/locus-addon-tasker/blob/9a2b3c8607d7d07bb8a9bb6fd4c0fb14c26e2692/app/src/main/java/falcosc/locus/addon/tasker/uc/UpdateContainerFieldFactory.java#L75

And here is how the user request gets a new update container and how it executes the getter function via apply to get the value https://github.com/Falcosc/locus-addon-tasker/blob/9a2b3c8607d7d07bb8a9bb6fd4c0fb14c26e2692/app/src/main/java/falcosc/locus/addon/tasker/intent/handler/UpdateContainerRequest.java#L45

I didn't debug step through the code but on the weekend I plan to go throug the API version to find the point at which it does break.

Values look like this
isEnabledMyLocation = false
isTrackRecRecording = false
getLocMyLocation().getLongitude() = 0.0
getLocMyLocation().getLongitude() was uninitialized
getLocMyLocation().getSpeed() 0.0

The project builds in the latest android studio. This is the change which breaks it: Falcosc/locus-addon-tasker@02ce313
I did comment out all API version specific implementations in this branch to highlite that only the API version change is causing the problem.

Do you already have a clue which refactoring could cause trouble with the update container?

@Falcosc Falcosc changed the title Update Container Fields are emtpy after updating from 0.9.39 to 0.9.62 in a Java 1.8 project Update Container Fields are emtpy after updating from 0.9.39 to 0.9.62 in a Java 1.8 project with Maps V4.27.1 Jan 20, 2025
@Falcosc
Copy link
Author

Falcosc commented Jan 23, 2025

Issue does not appear on Android 7, 0.9.62 works fine with Android 7

But does not work with Android 8 onwards.
Android 13 and 14 were tested as well.

Test setup is not equal, just random observations to provide clues.

@Falcosc
Copy link
Author

Falcosc commented Jan 23, 2025

Test results look like it broke with API 53
Locus_API_Android_0.9.52...Locus_API_Android_0.9.53

62 - no update container values
57 - no update container values
53 - no update container values
52 - all update container values work
51 - all update container values work
50 - all update container values work
39 - all update container values work

But I can't reproduce it with my physical android 7 device, it works fine even with 62.
And getting locus maps running in the emulator is a pain.

I hope you have enough clues or maybe it is easier for you to run https://github.com/Falcosc/locus-addon-tasker/tree/master in your IDE together with your locus project.
Let me know if you need a demo without tasker.
Locus Info requests work fine on the effected devices. Only Update Container are broken.

@menion
Copy link
Member

menion commented Jan 24, 2025

Hello Falco,
sorry for the extra work you have with this! Seems you updated the Locus API and did not adapt to the changed system in the Location object.

Now, most of the variables inside are 'nullable'. Previously, for example, "getAltitude" always returned any value even if altitude was not defined. Now it looks like this. altitude value is simply null in case, it is not valid.

Because your code is in Java and not Kotlin, you probably do not see a compile warning here.

May this be a problem you face here?

@menion menion self-assigned this Jan 24, 2025
@menion menion added the bug label Jan 24, 2025
@Falcosc
Copy link
Author

Falcosc commented Jan 24, 2025

Having null there is fine. Because here https://github.com/Falcosc/locus-addon-tasker/blob/9a2b3c8607d7d07bb8a9bb6fd4c0fb14c26e2692/app/src/main/java/falcosc/locus/addon/tasker/intent/handler/UpdateContainerRequest.java#L45 I just put the null result into String.valueOf which return "null" in this case.

And I actually got the warnings and changed the one place where null would cause issues. It only complained about places where the nullable altitude was put into null not supported annotated methods. But String.valueOf is supporting null.

The strange thing is: I can't reproduce it on my Android device. Currently, I have created a demo without any custom code to remove even more possible causes

private boolean exampleRequest() {
	try {
		Context appContext = getApplicationContext();
		UpdateContainer container = ActionBasics.INSTANCE.getUpdateContainer(appContext,
				Objects.requireNonNull(LocusUtils.INSTANCE.getActiveVersion(appContext, VersionCode.UPDATE_01)));
		Location loc = container.getLocMyLocation();
		Toast.makeText(this,  loc.getTime() + " Location: " + loc.getLatitude() + ", " + loc.getLongitude() + ' ' +
				loc.getLatitudeOriginal() + ", " + loc.getLongitudeOriginal(), Toast.LENGTH_LONG).show();
	} catch (Exception e) {
		Toast.makeText(this, "Error: " + e.getMessage(), Toast.LENGTH_LONG).show();
	}

	return true;
}

Now I wait for the response of the incompatible device users. Based on this example, we either get the cause of the issue as an error message or we get an easy to execute demo for you :)

@Falcosc
Copy link
Author

Falcosc commented Jan 24, 2025

Maybe we have trouble with the logging: since I can't reproduce it - I need to rely on the response of the incompatible device user.
But his Logcat outputs don't contain any API logs since it's a release build and I can't upload debugging true builds to google.

We can only see uncaught exceptions, but since everything is caught we don't see any errors.
@menion How could we get the API logging running on the device? Sideloading an unsigned debug build didn't work last week. Maybe my today's signed release apk with debugging true is able to be installed.

We did at least found an uncaught exception in locus.api.objects.styles.GeoDataStyle.readObject while switching from API 51 to 52 since it was uncaught, I will create a new ticket if it can be reproduced on 62 after we get the basics working. For that reason, we only test the gps location fields for now.
Since we get empty fields without exception, my first thought was a caught exception.

On prod we still use 0.9.39

Edit: we found a way, sideloading signed release APKs does work but now the error starts to disappear.

@Falcosc
Copy link
Author

Falcosc commented Jan 24, 2025

Only my non debugging release builds are affected, on my release builds I can reproduce the issue on all my devices, but now I have no logging support.

What kind of evil trick did you implement in your API to break only release builds? 😆

@Falcosc
Copy link
Author

Falcosc commented Jan 24, 2025

@menion I found the issue.

Starting with API 53, release builds are not able to get the update container.

Because my update container cache does init with new UpdateContainer() my app does not crash if I never get an update container from locus.

I did register a custom logging to log to TextView and got the error.
Starting with API 53 only my release builds throw this exception:

API: ActionBasics getUpdateContainer(android.app.Application@5b4af0f, menion.android.locus: 4.28.0)
java.lang.Class<locus.api.objects.extra.Location> has no zero argument constructor
java.io.IOException: java.lang.Class<locus.api.objects.extra.Location> has no zero argument constructor
	at locus.api.android.features.periodicUpdates.UpdateContainer.readLocation(SourceFile:50)
	at locus.api.android.features.periodicUpdates.UpdateContainer.readObject(SourceFile:23)
	at locus.api.objects.Storable.read(SourceFile:7)
	at locus.api.objects.Storable.read(SourceFile:2)
	at locus.api.android.ActionBasics.getUpdateContainer(SourceFile:43)
	at falcosc.locus.addon.tasker.MainActivity.exampleRequest(SourceFile:43)
	at falcosc.locus.addon.tasker.MainActivity$$ExternalSyntheticLambda1.onClick(SourceFile:16)
	at android.view.View.performClick(View.java:5685)
	at android.view.View$PerformClick.run(View.java:22481)
	at android.os.Handler.handleCallback(Handler.java:751)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:241)
	at android.app.ActivityThread.main(ActivityThread.java:6274)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

But I have trouble getting the source file lines corrected in the release build.
Even my own falcosc.locus.addon.tasker.MainActivity lines are still hidden

https://github.com/Falcosc/locus-addon-tasker/blob/APItest/app/proguard-rules.pro

If you tell me how to get your line numbers in the build, then I can repeat the test.
Otherwise, you could just check out https://github.com/Falcosc/locus-addon-tasker/tree/APItest
And after installation, you just need to tap on the large image in the center of the main screen. But it needs to be a release build to trigger the error.

@Falcosc Falcosc changed the title Update Container Fields are emtpy after updating from 0.9.39 to 0.9.62 in a Java 1.8 project with Maps V4.27.1 minifyEnabled stops working at API 0.9.53 onwards: Location has no zero argument constructor Jan 24, 2025
@menion
Copy link
Member

menion commented Jan 27, 2025

Thanks for the sample branch!

If you disable proguard/R8 with

minifyEnabled false
shrinkResources false

all seems to work correctly. So, it seems that the change in the location object caused the shrinker to be more aggressive.

Solution? Exclude Locus API objects with

# Keep Locus API objects
-keep class locus.api.objects.** { *; }

placed into "app/proguard-rules.pro" file.

Hope this helps.

@Falcosc
Copy link
Author

Falcosc commented Jan 27, 2025

Thank you
Should I create a section about this in your README as pull request?

@menion
Copy link
Member

menion commented Jan 27, 2025

I'll do this today, thanks for the offer.

Confirmation that this really solved this problem is welcome, thanks!

@Falcosc
Copy link
Author

Falcosc commented Jan 27, 2025

I still have trouble with keep, source file line numbers are still looking minified. But haven't deployed it yet.
Was the error reproduceable on your side and did disappear with the keep definition?

I have the same goal like https://issuetracker.google.com/issues/122752644#comment7
Just shrink resources stuff without messing with my stack traces.

The answer was

A clarification: the line number table is not removed. All the information is there for stack traces if you keep the line number table with keepattributes. We remap the line numbers to small integers to save space and save the mapping to real line numbers in the mapping file. You just have to use retrace to get back to original line numbers.

But I don't like to retrace all the time I see a stack trace.

@menion
Copy link
Member

menion commented Jan 27, 2025

I was testing it directly over Android Studio and yes, before > crash, after clean and second public > works ok.

Image

@Falcosc
Copy link
Author

Falcosc commented Jan 27, 2025

Perfect, thank you.

Meanwhile, I did give up and use progard option '-dontoptimize'
This allows having shrinkResources true while minifyEnabled true does not mess with my line numbers.

Falcosc/locus-addon-tasker@986792c

In the end, if your project uses lint to remove unused code, the optimization isn't worth the hassle but shrinkRessources is a meaningful addition:
shrinkRessources: from 4MB down to 2MB
dontoptimize: +70kb

Maybe you want to mention that dontoptimize is recommended to avoid the hassle with stacktraces while keeping shrinkRessources working.

We can close this with your readme update, thanks for your help.

@menion menion closed this as completed in 4f6565a Jan 27, 2025
@menion
Copy link
Member

menion commented Jan 27, 2025

Thanks, nice test.

Interesting is, that I do not have this "keep rule" in the main Locus Map app. On the second side, the Locus Map system is quite complex and it is possible, that the app keeps constructors of API objects for a different reason.

@Falcosc
Copy link
Author

Falcosc commented Jan 27, 2025

Do you run optimize on the whole Locus Maps project?

I assume Locus Maps does not convert binary into UpdateContainer objects. But yes, if you use more of them, less stuff gets removed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants