Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Microsoft.Android.Sdk.ILLink] fix crash when TZ changes #7956

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Apr 12, 2023

Fixes: #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 Author

Ok new test failed as expected:

04-12 22:57:44.275 11569 11569 I XALINKERTESTS: [FAIL] PreserveTest.MethodsArePreserved FAILED: System.NullReferenceException: Object reference not set to an instance of an object
04-12 22:57:44.275 11569 11569 I XALINKERTESTS:    at PreserveTest.MethodsArePreserved()

Let me change it so it isn't an NRE, and then put the fix in.

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 jonathanpeppers marked this pull request as ready for review April 13, 2023 15:44
@jonathanpeppers
Copy link
Member Author

Looks like the new test is green now, the only test failure is odd, but probably unrelated:

                   (_Upload target) -> 
                     /Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.0-ci.pr.gh7956.249/tools/Xamarin.Android.Common.Debugging.targets(623,5): error XA0137: The 'run-as' command failed with 'run-as: couldn't stat /data/user/11/com.xamarin.applicationrunswithdebuggerandbreaks: No such file or directory'. [/Users/runner/work/1/a/TestRelease/04-13_14.33.17/temp/ApplicationRunsWithDebuggerAndBreaksFalseAssembliesFalseguest1/App/App.csproj]
                   /Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.0-ci.pr.gh7956.249/tools/Xamarin.Android.Common.Debugging.targets(623,5): error XA0137: Fast Deployment is not currently supported on this device. [/Users/runner/work/1/a/TestRelease/04-13_14.33.17/temp/ApplicationRunsWithDebuggerAndBreaksFalseAssembliesFalseguest1/App/App.csproj]
                   /Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.0-ci.pr.gh7956.249/tools/Xamarin.Android.Common.Debugging.targets(623,5): error XA0137: Please file an issue with the exact error message using the 'Help->Send Feedback->Report a Problem' menu item in Visual Studio [/Users/runner/work/1/a/TestRelease/04-13_14.33.17/temp/ApplicationRunsWithDebuggerAndBreaksFalseAssembliesFalseguest1/App/App.csproj]
                   /Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.0-ci.pr.gh7956.249/tools/Xamarin.Android.Common.Debugging.targets(623,5): error XA0137: or 'Help->Report a Problem' in Visual Studio for Mac. [/Users/runner/work/1/a/TestRelease/04-13_14.33.17/temp/ApplicationRunsWithDebuggerAndBreaksFalseAssembliesFalseguest1/App/App.csproj]
                   /Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.0-ci.pr.gh7956.249/tools/Xamarin.Android.Common.Debugging.targets(623,5): error XA0137: Please set the 'EmbedAssembliesIntoApk' MSBuild property to 'true' to disable Fast Deployment in the Visual Studio project property pages, or edit the project file in a text editor. [/Users/runner/work/1/a/TestRelease/04-13_14.33.17/temp/ApplicationRunsWithDebuggerAndBreaksFalseAssembliesFalseguest1/App/App.csproj]

@jonpryor jonpryor merged commit 59e1e46 into dotnet:main Apr 13, 2023
@jonathanpeppers jonathanpeppers deleted the timezone-crash branch April 13, 2023 18:03
grendello added a commit to grendello/xamarin-android that referenced this pull request Apr 14, 2023
* main:
  Bump to xamarin/Java.Interop/main@554d819 (dotnet#7951)
  [Microsoft.Android.Sdk.ILLink] fix crash when TZ changes (dotnet#7956)
  [tests] Port 'Xamarin.Android.JcwGen-Tests.JcwGen-Tests' to .NET (dotnet#7949)
  [Xamarin.Android.Build.Tasks] remove `pdb2mdb` (dotnet#7950)
  [ci] Add some extra params to configure the test templates (dotnet#7955)
  Convert `/tools` and `/build-tools` projects from `net472` to `$(DotNetStableTargetFramework)` (dotnet#7943)
  [Xamarin.Android.Build.Tasks] fix cases of missing `@(Reference)` (dotnet#7947)
  Bump com.android.tools:r8 from 4.0.52 to 8.0.40 (dotnet#7934)
  Bump to xamarin/Java.Interop/main@a172402 (dotnet#7944)
  [Xamarin.Android] Remove OpenTK, sqlite-xamarin, System.EnterpriseServices. (dotnet#7940)
  [ci] Stop building classic test suites. (dotnet#7938)
  Bumping to the correct monodroid commit
  Trying to bump monodroid to run debugger-tests
  Pass timeout to runtime
grendello added a commit to grendello/xamarin-android that referenced this pull request Apr 17, 2023
* main:
  Bump to xamarin/Java.Interop/main@554d819 (dotnet#7951)
  [Microsoft.Android.Sdk.ILLink] fix crash when TZ changes (dotnet#7956)
  [tests] Port 'Xamarin.Android.JcwGen-Tests.JcwGen-Tests' to .NET (dotnet#7949)
  [Xamarin.Android.Build.Tasks] remove `pdb2mdb` (dotnet#7950)
  [ci] Add some extra params to configure the test templates (dotnet#7955)
jonathanpeppers added a commit that referenced this pull request 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.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing timezone crashes app
3 participants