Skip to content

Commit 60363ef

Browse files
grendellojonpryor
grendello
authored and
jonpryor
committed
Optionally wrap Java exceptions in BCL exceptions (#3855)
Fixes: #3841 Context: #3646 There are many types provided by Xamarin.Android which subclass Base Class Library (BCL) types or implement BCL interfaces, such as `Android.Runtime.InputStreamInvoker`, which inherits `System.IO.Stream`, or `Android.Runtime.JavaDictionary`, which implements `System.Collections.IDictionary`. Unfortunately, there is a long-standing bug in all of these types: they don't conform to BCL documentation regarding which exception types may be thrown. For example, `Stream.Read()` is documented as potentially throwing `System.IO.IOException` in certain circumstances. `InputStreamInvoker.Read()`, meanwhile, will *never* throw `System.IO.IOException` but *can* throw `Java.IO.IOException` (which does not and cannot inherit `System.IO.IOException`). The result is that if you have code which expects documented BCL semantics, e.g. throws `System.IO.IOException`, that code *may not work as expected* when running within Xamarin.Android. This is particularly problematic if such code is e.g. a .NET Standard library, in which case it *cannot* know about Java exception types. (It could catch `System.Exception`, but this may be undesirable.) Furthermore, "just fixing" the Xamarin.Android types so that they properly conform to documented exception behavior means that *existing* Xamarin.Android apps may break, as they may be catching the (currently thrown) Java types but not the BCL types. This makes for a [catch-22][0]: either way we go, *somebody* breaks. Begin to square this circle: 1. Introduce a new `$(AndroidBoundExceptionType)` MSBuild property to control which set of exception types may be thrown from methods overriding or implementing .NET methods, and 2. Publicly document when the default value will change. The `$(AndroidBoundExceptionType)` MSBuild property accepts one of two possible string values: * `Java`: Certain Xamarin.Android methods which override or implement BCL methods may throw `Java.Lang.Throwable`-derived instances. For example, `InputStreamInvoker.Read()` may throw `Java.IO.IOException`. * `System`: Certain Xamarin.Android methods which override or implement BCL methods will *not* throw `Java.Lang.Throwable`- derived instances. In this case, the thrown exception will contain the original Java exception in the `Exception.InnerException` property. For example, `InputStreamInvoker.Read()` may throw `new System.IO.IOException(e.Message, e)`, where `e` is a `Java.IO.IOException`. In Xamarin.Android 10.5 (to be released with Visual Studio 16.5), the default value will be `Java`. This is semantically consistent with all previous Xamarin.Android releases. [When .NET 5 is released with Xamarin.Android support][1], the default value will be `System`. This is anticipated to be Fall 2020. Note in particular the "methods which override or implement BCL methods" wording: we will *not* alter the exception types which may be thrown from e.g. [`Activity.SystemorResult()`][2], as that method doesn't override or implement a BCL method. [0]: https://en.wikipedia.org/wiki/Catch-22#Concept [1]: https://devblogs.microsoft.com/dotnet/introducing-net-5/ [2]: https://developer.android.com/reference/android/app/Activity.html#startActivityForResult(android.content.Intent,%20int)
1 parent a7f5cb2 commit 60363ef

22 files changed

+1321
-155
lines changed

‎Documentation/guides/BuildProcess.md‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,35 @@ when packaging Release applications.
290290
291291
[binutils]: https://android.googlesource.com/toolchain/binutils/
292292
293+
- **AndroidBoundExceptionType** – A string value which specifies how
294+
exceptions should be propagated when a Xamarin.Android-provided type
295+
implements a .NET type or interface in terms of Java types, for example
296+
`Android.Runtime.InputStreamInvoker` and `System.IO.Stream`, or
297+
`Android.Runtime.JavaDictionary` and `System.Collections.IDictionary`.
298+
299+
- `Java`: The original Java exception type is propagated as-is.
300+
301+
This means that e.g. `InputStreamInvoker` does not property implement
302+
the `System.IO.Stream` API, as e.g. `Java.IO.IOException` may be thrown
303+
from `Stream.Read()` instead of `System.IO.IOException`.
304+
305+
This corresponds to exception propagation behavior in all releases of
306+
Xamarin.Android prior to 10.2.
307+
308+
This is the default value in Xamarin.Android 10.2.
309+
310+
- `System`: The original Java exception type is caught and wrapped in an
311+
appropriate .NET exception type.
312+
313+
This means that e.g. `InputStreamInvoker` properly implements
314+
`System.IO.Stream`, and `Stream.Read()` will *not* throw `Java.IO.IOException`
315+
instances. (It may instead throw a `System.IO.IOException` which has a
316+
`Java.IO.IOException` as the `Exception.InnerException` value.)
317+
318+
This will become the default value in Xamarin.Android 11.0.
319+
320+
Added in Xamarin.Android 10.2.
321+
293322
- **AndroidBuildApplicationPackage** – A boolean value that
294323
indicates whether to create and sign the package (.apk). Setting
295324
this value to `True` is equivalent to using the

‎Documentation/release-notes/master.md‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
* [Template for new features](#template-for-new-features)
88
* [Build and deployment performance](#build-and-deployment-performance)
99
* [App startup performance](#app-startup-performance)
10+
* [Java exception wrapping](#java-exception-wrapping)
1011
* [Issues fixed](#issues-fixed)
1112

1213
### Template for new features
@@ -61,6 +62,19 @@ these methods/fields.
6162

6263
### App startup performance
6364

65+
### Java exception wrapping
66+
67+
`Mono.Android.dll` has been audited to find code which can throw Java exceptions in context where throwing a .NET exception
68+
would be more logical/expected. `Xamarin.Android`'s has always thrown the Java exceptions, however it poses a problem for
69+
applications which have most of their code stored in portable assemblies that do not reference `Mono.Android` as they are unable
70+
to catch and handle the Java exceptions. The simplest solution - to always wrap Java exceptions in BCL ones - is unfortunately
71+
not viable because of backward compatibility. For this release the old behavior remains the default, but one can opt in to the
72+
new wrapping behavior by setting the `$(AndroidBoundExceptionType)` MSBuild property to `System`. After this is done, all the
73+
**documented** Java exceptions will be wrapped in corresponding BCL exceptions (with the original exception placed in the
74+
`InnerException` property and the original message reused in the wrapper exception).
75+
Note that this wrapping happens **only** for Java/Android types which use the **.NET** classes (e.g. wherever Java.IO.InputStream
76+
is used by the Android class but `Xamarin.Android` binds it as `System.IO.Stream`)
77+
6478
### Issues fixed
6579

6680
* [GitHub NNNN](https://github.com/xamarin/xamarin-android/issues/NNNN):

‎src/Mono.Android/Android.Runtime/AndroidEnvironment.cs‎

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,17 @@ internal static void UnhandledException (Exception e)
136136
// This is invoked by
137137
// System.dll!System.AndroidPlatform.TrustEvaluateSsl()
138138
// DO NOT REMOVE
139+
//
140+
// Exception audit:
141+
//
142+
// Verdict
143+
// No need to wrap thrown exceptions in a BCL class
144+
//
145+
// Rationale
146+
// This method is called by System.AndroidPlatform.TrustEvaluateSsl which is, eventually, called by
147+
// System/Mono.Net.Security/SystemCertificateValidator(). All exceptions are caught and handled
148+
// by the caller.
149+
//
139150
static bool TrustEvaluateSsl (List <byte[]> certsRawData)
140151
{
141152
SetupTrustManager ();
@@ -172,6 +183,17 @@ static bool TrustEvaluateSsl (List <byte[]> certsRawData)
172183
// This is invoked by
173184
// System.dll!!System.AndroidPlatform.CertStoreLookup()
174185
// DO NOT REMOVE
186+
//
187+
// Exception audit:
188+
//
189+
// Verdict
190+
// No need to wrap thrown exceptions in a BCL class
191+
//
192+
// Rationale
193+
// This method is called by System.AndroidPlatform.CertStoreLookup which is, eventually, called by
194+
// System.Mono.Btls.MonoBtlsX509LookupMono.OnGetBySubject(). All exceptions are caught and handled
195+
// by the caller.
196+
//
175197
static byte[] CertStoreLookup (long hash, bool userStore)
176198
{
177199
SetupCertStore ();
@@ -219,6 +241,14 @@ static void NotifyTimeZoneChanged ()
219241
// This is invoked by
220242
// System.Drawing!System.Drawing.GraphicsAndroid.FromAndroidSurface ()
221243
// DO NOT REMOVE
244+
//
245+
// Exception audit:
246+
//
247+
// Verdict
248+
// No need to wrap thrown exceptions in a BCL class
249+
//
250+
// Rationale
251+
// No longer called by the indicated caller, however we keep it for backward compatibility.
222252
static void GetDisplayDPI (out float x_dpi, out float y_dpi)
223253
{
224254
var wm = Application.Context.GetSystemService (Context.WindowService).JavaCast <IWindowManager> ();
@@ -235,6 +265,16 @@ static void GetDisplayDPI (out float x_dpi, out float y_dpi)
235265
// This is invoked by
236266
// System.Core!System.AndroidPlatform.GetDefaultTimeZone ()
237267
// DO NOT REMOVE
268+
//
269+
// Exception audit:
270+
//
271+
// Verdict
272+
// No need to wrap thrown exceptions in a BCL class
273+
//
274+
// Rationale
275+
// Java code (invoked from our native runtime) will always return the default timezone and no
276+
// exceptions are documented for the Java API.
277+
//
238278
static string GetDefaultTimeZone ()
239279
{
240280
IntPtr id = _monodroid_timezone_get_default_id ();
@@ -254,8 +294,12 @@ static string GetDefaultTimeZone ()
254294
static SynchronizationContext GetDefaultSyncContext ()
255295
{
256296
var looper = Android.OS.Looper.MainLooper;
257-
if (Android.OS.Looper.MyLooper() == looper)
258-
return Android.App.Application.SynchronizationContext;
297+
try {
298+
if (Android.OS.Looper.MyLooper() == looper)
299+
return Android.App.Application.SynchronizationContext;
300+
} catch (System.Exception ex) {
301+
Logger.Log (LogLevel.Warn, "MonoAndroid", $"GetDefaultSyncContext caught a Java exception: {ex}");
302+
}
259303
return null;
260304
}
261305

@@ -328,9 +372,21 @@ static object GetHttpMessageHandler ()
328372
class _Proxy : IWebProxy {
329373
readonly ProxySelector selector = ProxySelector.Default;
330374

375+
// Exception audit:
376+
//
377+
// Verdict
378+
// Exception wrapping required
379+
//
380+
// Rationale
381+
// Java code may throw URISyntaxException which we map to the managed UriFormatException
382+
//
331383
static URI CreateJavaUri (Uri destination)
332384
{
333-
return new URI (destination.Scheme, destination.UserInfo, destination.Host, destination.Port, destination.AbsolutePath, destination.Query, destination.Fragment);
385+
try {
386+
return new URI (destination.Scheme, destination.UserInfo, destination.Host, destination.Port, destination.AbsolutePath, destination.Query, destination.Fragment);
387+
} catch (Java.Lang.Throwable ex) when (JNIEnv.ShouldWrapJavaException (ex)) {
388+
throw new UriFormatException (ex.Message, ex);
389+
}
334390
}
335391

336392
public Uri GetProxy (Uri destination)

‎src/Mono.Android/Android.Runtime/BoundExceptionType.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
namespace Android.Runtime
2+
{
3+
// Keep the enum values in sync with those in src/monodroid/jni/monodroid-glue-internal.hh
4+
enum BoundExceptionType : byte
5+
{
6+
System = 0x00,
7+
Java = 0x01,
8+
};
9+
}

‎src/Mono.Android/Android.Runtime/InputStreamInvoker.cs‎

Lines changed: 60 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,53 @@ public class InputStreamInvoker : Stream
99

1010
public InputStreamInvoker (Java.IO.InputStream stream)
1111
{
12+
if (stream == null)
13+
throw new ArgumentNullException (nameof (stream));
14+
1215
this.BaseInputStream = stream;
1316
}
1417

18+
//
19+
// Exception audit:
20+
//
21+
// Verdict
22+
// Exception wrapping is required.
23+
//
24+
// Rationale
25+
// `java.io.InputStream.close()` throws an exception, see:
26+
//
27+
// https://developer.android.com/reference/java/io/InputStream?hl=en
28+
//
1529
protected override void Dispose (bool disposing)
1630
{
1731
if (disposing && BaseInputStream != null) {
18-
BaseInputStream.Dispose ();
19-
BaseInputStream = null;
32+
try {
33+
BaseInputStream.Close ();
34+
BaseInputStream.Dispose ();
35+
BaseInputStream = null;
36+
} catch (Java.IO.IOException ex) when (JNIEnv.ShouldWrapJavaException (ex)) {
37+
throw new IOException (ex.Message, ex);
38+
}
39+
}
40+
}
41+
42+
//
43+
// Exception audit:
44+
//
45+
// Verdict
46+
// Exception wrapping is required.
47+
//
48+
// Rationale
49+
// `java.io.InputStream.close()` throws an exception, see:
50+
//
51+
// https://developer.android.com/reference/java/io/InputStream?hl=en
52+
//
53+
public override void Close ()
54+
{
55+
try {
56+
BaseInputStream.Close ();
57+
} catch (Java.IO.IOException ex) when (JNIEnv.ShouldWrapJavaException (ex)) {
58+
throw new IOException (ex.Message, ex);
2059
}
2160
}
2261

@@ -25,9 +64,27 @@ public override void Flush ()
2564
// No need to flush an input stream
2665
}
2766

67+
//
68+
// Exception audit:
69+
//
70+
// Verdict
71+
// Exception wrapping is required.
72+
//
73+
// Rationale
74+
// `java.io.InputStream.read(byte[], int, int)` throws an exception, see:
75+
//
76+
// https://developer.android.com/reference/java/io/InputStream?hl=en#read(byte%5B%5D,%20int,%20int)
77+
//
2878
public override int Read (byte[] buffer, int offset, int count)
2979
{
30-
int res = BaseInputStream.Read (buffer, offset, count);
80+
int res;
81+
82+
try {
83+
res = BaseInputStream.Read (buffer, offset, count);
84+
} catch (Java.IO.IOException ex) when (JNIEnv.ShouldWrapJavaException (ex)) {
85+
throw new IOException (ex.Message, ex);
86+
}
87+
3188
if (res == -1)
3289
return 0;
3390
return res;
@@ -76,4 +133,3 @@ public static Stream FromJniHandle (IntPtr handle, JniHandleOwnership transfer)
76133
}
77134
}
78135
}
79-

‎src/Mono.Android/Android.Runtime/JNIEnv.cs‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Linq;
66
using System.Linq.Expressions;
77
using System.Reflection;
8+
using System.Runtime.CompilerServices;
89
using System.Runtime.ExceptionServices;
910
using System.Runtime.InteropServices;
1011
using System.Threading;
@@ -14,7 +15,7 @@
1415
using Java.Interop.Tools.TypeNameMappings;
1516

1617
namespace Android.Runtime {
17-
18+
#pragma warning disable 0649
1819
struct JnienvInitializeArgs {
1920
public IntPtr javaVm;
2021
public IntPtr env;
@@ -31,7 +32,9 @@ struct JnienvInitializeArgs {
3132
public int isRunningOnDesktop;
3233
public byte brokenExceptionTransitions;
3334
public int packageNamingPolicy;
35+
public byte ioExceptionType;
3436
}
37+
#pragma warning restore 0649
3538

3639
public static partial class JNIEnv {
3740
static IntPtr java_class_loader;
@@ -56,6 +59,7 @@ public static partial class JNIEnv {
5659
internal static bool IsRunningOnDesktop;
5760

5861
static AndroidRuntime androidRuntime;
62+
static BoundExceptionType BoundExceptionType;
5963

6064
[DllImport ("__Internal", CallingConvention = CallingConvention.Cdecl)]
6165
extern static void monodroid_log (LogLevel level, LogCategories category, string message);
@@ -88,6 +92,25 @@ internal static bool IsGCUserPeer (IntPtr value)
8892
return IsInstanceOf (value, grefIGCUserPeer_class);
8993
}
9094

95+
internal static bool ShouldWrapJavaException (Java.Lang.Throwable t, [CallerMemberName] string caller = null)
96+
{
97+
if (t == null) {
98+
monodroid_log (LogLevel.Warn,
99+
LogCategories.Default,
100+
$"ShouldWrapJavaException was not passed a valid `Java.Lang.Throwable` instance. Called from method `{caller}`");
101+
return false;
102+
}
103+
104+
bool wrap = BoundExceptionType == BoundExceptionType.System;
105+
if (!wrap) {
106+
monodroid_log (LogLevel.Warn,
107+
LogCategories.Default,
108+
$"Not wrapping exception of type {t.GetType().FullName} from method `{caller}`. This will change in a future release.");
109+
}
110+
111+
return wrap;
112+
}
113+
91114
[DllImport ("libc")]
92115
static extern int gettid ();
93116

@@ -141,6 +164,7 @@ internal static unsafe void Initialize (JnienvInitializeArgs* args)
141164

142165
Mono.SystemDependencyProvider.Initialize ();
143166

167+
BoundExceptionType = (BoundExceptionType)args->ioExceptionType;
144168
androidRuntime = new AndroidRuntime (args->env, args->javaVm, androidSdkVersion > 10, args->grefLoader, args->Loader_loadClass);
145169

146170
AllocObjectSupported = androidSdkVersion > 10;

0 commit comments

Comments
 (0)