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

Changing timezone crashes app #7953

Closed
rgroenewoudt opened this issue Apr 12, 2023 · 9 comments · Fixed by #7956
Closed

Changing timezone crashes app #7953

rgroenewoudt opened this issue Apr 12, 2023 · 9 comments · Fixed by #7956
Assignees
Labels
Area: Linker Issues when linking assemblies.

Comments

@rgroenewoudt
Copy link

rgroenewoudt commented Apr 12, 2023

Android application type

.NET Android (net7.0-android, etc.)

Affected platform version

VS2022 17.5.4, .NET 7 Android

Description

When building an app with PublishTrimmed enabled, changing the device timezone crashes the app:

[monodroid] Unable to find Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()!

illinker seems to remove the method.

The timezone is often automatically changed by getting close to an other country, even though the time itself doesn't change. (changes to Europe/Amsterdam to Europe/Berlin)

The timezone can also be changed manually in the Android settings for the same effect.

Steps to Reproduce

Attached is a new Android 7 app with PublishTrimmed enabled in debug mode which shows the started timestamp in UI.

AndroidApp1.zip

Start the app from Visual Studio in debug mode and note the time. Go to Android settings and change the current timezone to for example Afghanistan.
The app immediately crashes, VS stops debugging and the log shows the error message that AndroidEnvironment.NotifyTimeZoneChanged() is missing.

This happens in both emulator-33 and physical devices.

Did you find any workaround?

Add a linker description file containing:

<linker>
	<assembly fullname="Mono.Android">
		<type fullname="Android.Runtime.AndroidEnvironment">
			<method name="NotifyTimeZoneChanged" /> <!-- Workaround for Xamarin bug trimming method away -->
		</type>
	</assembly>
</linker>

To prevent the method being trimmed away.

Relevant log output

No response

@rgroenewoudt rgroenewoudt added Area: App Runtime Issues in `libmonodroid.so`. needs-triage Issues that need to be assigned. labels Apr 12, 2023
@rgroenewoudt
Copy link
Author

I would also like to add that this was very difficult to debug.
The message [monodroid] Unable to find Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()! only shows up in debug mode and not in release mode.
Nothing in adb logcat or adb bugreports.

Android's ActivityManager.GetHistoricalProcessExitReasons() only reports REASON_EXIT_SELF with signal 13. (it's not recognized as crash?)

If the app is immediately killed/aborted, I would at least expect the related error message to be present in logcat.

@grendello grendello added Area: Linker Issues when linking assemblies. and removed Area: App Runtime Issues in `libmonodroid.so`. needs-triage Issues that need to be assigned. labels Apr 12, 2023
@grendello grendello removed their assignment Apr 12, 2023
@grendello
Copy link
Contributor

@rgroenewoudt The message is logged with the ANDROID_LOG_FATAL priority, can't get any higher alas. If it was missing from the logcat, then that's on Android I'm afraid.

@grendello
Copy link
Contributor

Android's ActivityManager.GetHistoricalProcessExitReasons() only reports REASON_EXIT_SELF with signal 13. (it's not recognized as crash?)

Back in the day, calling exit() from the application would actually terminate the process. At some point Android changed the behavior and right now it mostly restarts the application if exit() is called. To quit the app, one needs to call abort() now. Until very recently, we weren't consistent in that, preserving a lot of older code that still called exit() on fatal failures.

@jonathanpeppers
Copy link
Member

@grendello do we need to do an audit of all mono_class_get_method_from_name calls and make sure we have xml to preserve it? Some might be fine to trim away, if we don't have to abort in those cases.

I will work on fixing this specific case -- adding a test. I think I can probably just try calling the method from System.Reflection in Release mode to assert that it exists.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Apr 12, 2023
Fixes: dotnet#7953

**WIP** seeing what the test does on CI.

When a timezone changes in a `Release` app, it can crash with:

    [monodroid] Unable to find Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()!

In 11f0e1b, we removed the line:

    <?xml version="1.0" encoding="utf-8" ?>
    <linker>
        <assembly fullname="Mono.Android">
    --      <type fullname="Android.Runtime.AndroidEnvironment" />

Unfortunately, this method is called from native code, so we need to
*always* preserve it.

Added a test for this scenario, we may want to audit all
`mono_class_get_method_from_name` calls and add more tests cases.

I also cleaned up the tests a bit with a `getResource()` local function.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Apr 13, 2023
Fixes: dotnet#7953

When a timezone changes in a `Release` app, it can crash with:

    [monodroid] Unable to find Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()!

In 11f0e1b, we removed the line:

    <?xml version="1.0" encoding="utf-8" ?>
    <linker>
        <assembly fullname="Mono.Android">
    --      <type fullname="Android.Runtime.AndroidEnvironment" />

Unfortunately, this method is called from native code, so we need to
*always* preserve it.

Added a test for this scenario, we may want to audit all
`mono_class_get_method_from_name` calls and add more tests cases.

I also cleaned up the tests a bit with a `getResource()` local function.
@grendello
Copy link
Contributor

@jonathanpeppers Isn't it a bug in the linker though? I was under impression that such lines preserve the entire type? If not, then what's the point of the <type> element?

@grendello
Copy link
Contributor

@grendello do we need to do an audit of all mono_class_get_method_from_name calls and make sure we have xml to preserve it? Some might be fine to trim away, if we don't have to abort in those cases.

If the <type> element doesn't preserve the entire type, then yes, we probably need to list each method in the preserve lists.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Apr 13, 2023
Fixes: dotnet#7953

When a timezone changes in a `Release` app, it can crash with:

    [monodroid] Unable to find Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()!

In 11f0e1b, we removed the line:

    <?xml version="1.0" encoding="utf-8" ?>
    <linker>
        <assembly fullname="Mono.Android">
    --      <type fullname="Android.Runtime.AndroidEnvironment" />

Unfortunately, this method is called from native code, so we need to
*always* preserve it.

Added a test for this scenario, we may want to audit all
`mono_class_get_method_from_name` calls and add more tests cases.

I also cleaned up the tests a bit with a `getResource()` local function.
@jonathanpeppers
Copy link
Member

No, the problem here is we removed <type fullname="Android.Runtime.AndroidEnvironment" />: 11f0e1b

We don't actually have anything preserving AndroidEnvironment.NotifyTimeZoneChanged(), if it isn't called from managed code the linker doesn't know about it.

@rgroenewoudt
Copy link
Author

If only the .NET 5+ is supported, it might also be an idea to use DynamicallyAccessedMembersAttribute.

@jonathanpeppers
Copy link
Member

DynamicallyAccessedMembersAttribute would be good if using some type required this method, but I think all apps actually require it?

jonpryor pushed a commit that referenced this issue Apr 13, 2023
Fixes: #7953

Context: 11f0e1b

When a timezone changes in a `Release` config app, it can crash with:

	[monodroid] Unable to find Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()!

In commit 11f0e1b, we removed the line:

	<?xml version="1.0" encoding="utf-8" ?>
	<linker>
	    <assembly fullname="Mono.Android">
	--      <type fullname="Android.Runtime.AndroidEnvironment" />

Unfortunately, `AndroidEnvironment.NotifyTimeZoneChanged()` is called
from non-managed code, via:

  * The `NotifyTimeZoneChanges` broadcast receiver
    (in `src/java-runtime/java/mono/android/app/NotifyTimeZoneChanges.java`)
    which calls-
  * The `Rutime.notifyTimeZoneChanged()` `native` method
    (in `src/java-runtime/java/mono/android/Runtime.java`),
    implemented in-
  * `Java_mono_android_Runtime_notifyTimeZoneChanged()`
    (in`src/monodroid/jni/timezones.cc`).

The managed linker cannot "see" any of these references, so we
need to *always* preserve this method, as it is always callable.

Update `src/Microsoft.Android.Sdk.ILLink/PreserveLists/Mono.Android.xml`
so that `Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()`
is always preserved.

Added a test for this scenario.

TODO: we may want to audit all `mono_class_get_method_from_name()`
calls and add more tests cases.

I also cleaned up the tests a bit with a `getResource()` local function.
jonathanpeppers added a commit that referenced this issue Apr 25, 2023
Fixes: #7953

Context: 11f0e1b

When a timezone changes in a `Release` config app, it can crash with:

	[monodroid] Unable to find Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()!

In commit 11f0e1b, we removed the line:

	<?xml version="1.0" encoding="utf-8" ?>
	<linker>
	    <assembly fullname="Mono.Android">
	--      <type fullname="Android.Runtime.AndroidEnvironment" />

Unfortunately, `AndroidEnvironment.NotifyTimeZoneChanged()` is called
from non-managed code, via:

  * The `NotifyTimeZoneChanges` broadcast receiver
    (in `src/java-runtime/java/mono/android/app/NotifyTimeZoneChanges.java`)
    which calls-
  * The `Rutime.notifyTimeZoneChanged()` `native` method
    (in `src/java-runtime/java/mono/android/Runtime.java`),
    implemented in-
  * `Java_mono_android_Runtime_notifyTimeZoneChanged()`
    (in`src/monodroid/jni/timezones.cc`).

The managed linker cannot "see" any of these references, so we
need to *always* preserve this method, as it is always callable.

Update `src/Microsoft.Android.Sdk.ILLink/PreserveLists/Mono.Android.xml`
so that `Android.Runtime.AndroidEnvironment.NotifyTimeZoneChanged()`
is always preserved.

Added a test for this scenario.

TODO: we may want to audit all `mono_class_get_method_from_name()`
calls and add more tests cases.

I also cleaned up the tests a bit with a `getResource()` local function.
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Area: Linker Issues when linking assemblies.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants