Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Aug 30, 2017

Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=56819

Bumps to Java.Interop/master/ab3c2b26.

Nothing prevents "anybody" from custom implementing IJavaObject:

class MyBadClass : Android.Runtime.IJavaObject {
    public IntPtr Handle {
        get {return IntPtr.Zero;}
    }

    public void Dispose () {}
}

The problem is that the above doesn't actually work: the
IJavaObject.Handle value contains the JNI Object Reference to pass
into JNI. The above code will thus result in always passing null
into Java code, which could result in a NullPointerException, but
will always result in not doing what was intended.

While it is theoretically possible to manually implement
IJavaObject, it's not something I would want to contemplate, nor
is it for the faint of heart, so long ago we decided to emit a warning
if we encounter a type which:

  1. Implements Android.Runtime.IJavaObject, and
  2. Does not also inherit Java.Lang.Object or
    Java.Lang.Throwable.

As such, the above MyBadClass elicits the warning:

Type 'MyBadClass' implements Android.Runtime.IJavaObject but does not inherit from Java.Lang.Object. It is not supported.

This is all well and good, but (effectively) nobody reads warnings,
especially since our toolchain emits so many warnings that enabling
/warnaserror is for the truly foolhardy, so lots of warnings is,
unfortunately, normal.

Which brings us to Bug #56819: Could we make this an error?

Answer: Of course we can. That said, I have no idea how much existing
code this will break if we turned it into an error. That might be
good and useful, but if there's no way to disable the error,
existing apps which (appear to) work will no longer build.

That's not desirable.

Add a new $(AndroidErrorOnCustomJavaObject) MSBuild property to
control what happens when such custom IJavaObject implementations
are found. When True, the default, emit a new XA4212 error.
The error can be turned back into a warning by setting
$(AndroidErrorOnCustomJavaObject) within the App.csproj:

<!-- Within App.csproj -->
<PropertyGroup>
  <AndroidErrorOnCustomJavaObject>False</AndroidErrorOnCustomJavaObject>
</PropertyGroup>

@jonpryor jonpryor requested a review from dellis1972 August 30, 2017 16:34
@jonpryor jonpryor force-pushed the jonp-custom-IJavaObject-is-an-error branch from 501396f to 107fa1c Compare August 30, 2017 16:36
: Path.Combine (Environment.GetFolderPath (Environment.SpecialFolder.LocalApplicationData), CacheBaseDir);

using (var resolver = new DirectoryAssemblyResolver (LogWarning, loadDebugSymbols: false)) {
using (var resolver = new DirectoryAssemblyResolver (Log.CreateTaskLogger (), loadDebugSymbols: false)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonpryor not sure we can use it here. Doing that in any Task which uses AsyncTask will lock the UI in VS because we cannot use Log on a background thread. So that code was probably an accident waiting to happen anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably have an overload which takes the AsyncTask and uses the LogError and LogWarning methods on that class.

@jonpryor jonpryor force-pushed the jonp-custom-IJavaObject-is-an-error branch 2 times, most recently from 83fd11f to 3476b88 Compare August 30, 2017 17:41
@jonpryor jonpryor changed the title [Xamarin.Android.Build.Tasks] Add $(AndroidErrorOnCustomJavaObject). [Xamarin.Android.Build.Tasks] Add $(AndroidErrorOnCustomJavaObject) Aug 30, 2017
@jonpryor jonpryor force-pushed the jonp-custom-IJavaObject-is-an-error branch from 3476b88 to e9fc3e3 Compare August 30, 2017 18:31
@jonpryor
Copy link
Contributor Author

NOW WITH UNIT TESTS

Which actually found several typo's....

Fixes:  https://bugzilla.xamarin.com/show_bug.cgi?id=56819

Bumps to Java.Interop/master/ab3c2b26.

Nothing *prevents* "anybody" from custom implementing `IJavaObject`:

	class MyBadClass : Android.Runtime.IJavaObject {
	    public IntPtr Handle {
	        get {return IntPtr.Zero;}
	    }

	    public void Dispose () {}
	}

The problem is that the above doesn't actually work: the
`IJavaObject.Handle` value contains the JNI Object Reference to pass
into JNI. The above code will thus result in always passing `null`
into Java code, which could result in a `NullPointerException`, but
will always result in *not* doing what was intended.

While it is *theoretically* possible to manually implement
`IJavaObject`, it's not something *I* would want to contemplate, nor
is it for the faint of heart, so long ago we decided to emit a warning
if we encounter a type which:

 1. Implements `Android.Runtime.IJavaObject`, and
 2. Does *not* also inherit `Java.Lang.Object` or
    `Java.Lang.Throwable`.

As such, the above `MyBadClass` elicits the warning:

	Type 'MyBadClass' implements Android.Runtime.IJavaObject but does not inherit from Java.Lang.Object. It is not supported.

This is all well and good, but (effectively) *nobody* reads warnings,
*especially* since our toolchain emits so many warnings that enabling
`/warnaserror` is for the truly foolhardy, so *lots* of warnings is,
unfortunately, normal.

Which brings us to Bug #56819: Could we make this an error?

Answer: Of course we can. That said, I have no idea how much existing
code this will *break* if we turned it into an error. That might be
good and useful, but if there's no way to *disable* the error,
existing apps which (appear to) work will no longer build.

That's not desirable.

Add a new `$(AndroidErrorOnCustomJavaObject)` MSBuild property to
control what happens when such custom `IJavaObject` implementations
are found. When True, the default, emit a new XA4212 *error*.
The error can be turned back into a warning by setting
`$(AndroidErrorOnCustomJavaObject)` within the `App.csproj`:

	<!-- Within App.csproj -->
	<PropertyGroup>
	  <AndroidErrorOnCustomJavaObject>False</AndroidErrorOnCustomJavaObject>
	</PropertyGroup>
@jonpryor jonpryor force-pushed the jonp-custom-IJavaObject-is-an-error branch from e9fc3e3 to 38bc568 Compare August 30, 2017 18:38
@dellis1972 dellis1972 merged commit 9d131af into dotnet:master Aug 30, 2017
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Aug 31, 2017
Commit 9d131af introduced a [unit test failure][m573] on
xamarin-android/master:

[m573]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/573/

	Xamarin.Android.Build.Tests.BuildTest.XA4212
	Expected: String containing "warning XA4"
	 But was: <log file contents...>

Aside: The XA4212 test creates a project with a bad `IJavaObject`
type, builds the project -- which triggers an XA4212 error, which *is*
observed in the unit test -- then *rebuilds* the project, but this
time with `$(AndroidErrorOnCustomJavaObject)`=False.

The intention is that the second build should instead elicit an XA4212
*warning*, but otherwise continue just fine.

What's interesting about the log file contents is that it shows that
the `<GenerateJavaStubs/>` task isn't executed *at all*, but it's the
`<GenerateJavaStubs/>` task which is supposed to emit the warning!

	Skipping target "_GenerateJavaStubs" because its outputs are up-to-date.

Update the `<GenerateJavaStubs/>` task so that if it errors out, it
*removes* any `<GenerateJavaStubs/>` output files. This will ensure
that on subsequent builds, the `_GenerateJavaStubs` target won't be
inadvertently skipped.

(This issue *should* have been caught during the
[**macOS+xbuild PR builder** build for PR dotnet#797][pr1491]. It *wasn't*,
because the `Xamarin.Android.Build.Tests` tests weren't reported. For
example, PR dotnet#1491 ran 869 tests, while PR dotnet#1492 ran 1085! I'm not sure
what can be done to better detect and prevent this in the future.)

[pr1491]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-pr-builder/1491/
dellis1972 pushed a commit that referenced this pull request Aug 31, 2017
Commit 9d131af introduced a [unit test failure][m573] on
xamarin-android/master:

[m573]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/573/

	Xamarin.Android.Build.Tests.BuildTest.XA4212
	Expected: String containing "warning XA4"
	 But was: <log file contents...>

Aside: The XA4212 test creates a project with a bad `IJavaObject`
type, builds the project -- which triggers an XA4212 error, which *is*
observed in the unit test -- then *rebuilds* the project, but this
time with `$(AndroidErrorOnCustomJavaObject)`=False.

The intention is that the second build should instead elicit an XA4212
*warning*, but otherwise continue just fine.

What's interesting about the log file contents is that it shows that
the `<GenerateJavaStubs/>` task isn't executed *at all*, but it's the
`<GenerateJavaStubs/>` task which is supposed to emit the warning!

	Skipping target "_GenerateJavaStubs" because its outputs are up-to-date.

Update the `<GenerateJavaStubs/>` task so that if it errors out, it
*removes* any `<GenerateJavaStubs/>` output files. This will ensure
that on subsequent builds, the `_GenerateJavaStubs` target won't be
inadvertently skipped.

(This issue *should* have been caught during the
[**macOS+xbuild PR builder** build for PR #797][pr1491]. It *wasn't*,
because the `Xamarin.Android.Build.Tests` tests weren't reported. For
example, PR #1491 ran 869 tests, while PR #1492 ran 1085! I'm not sure
what can be done to better detect and prevent this in the future.)

[pr1491]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-pr-builder/1491/
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 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.

3 participants