Bug 59472 - CharacterSequence to c# string conversion is failing
Summary: CharacterSequence to c# string conversion is failing
Status: RESOLVED FIXED
Alias: None
Product: Android
Classification: Xamarin
Component: Bindings ()
Version: 8.0 (15.4)
Hardware: PC Mac OS
: Normal normal
Target Milestone: 15.6
Assignee: Atsushi Eno
URL:
Depends on:
Blocks:
 
Reported: 2017-09-13 19:59 UTC by Gonzalo Martin
Modified: 2017-10-23 15:01 UTC (History)
5 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Gonzalo Martin 2017-09-13 19:59:24 UTC
Something is failing in the CharacterSequence to c# string conversion. This is related with EmojiCompat sample.

##### Steps to reproduce #####
1. Download EmojiCompat sample https://github.com/gonzalonm/monodroid-samples/tree/android-o-emoji-sample/android-o/EmojiCompat
2. Build and run sample

##### Expected result #####
All fields are shown

##### Current result #####
One field (RegularTextView) is not shown due to a conversion problem.

This is the line: https://github.com/gonzalonm/monodroid-samples/blob/android-o-emoji-sample/android-o/EmojiCompat/EmojiCompat/MainActivity.cs#L60

Basically, the following line does not work as expected (using a c# string primitive):

> RegularTextView.Text = Get().Process(Text); // returns null

but this one is working fine (one possible workaround is use Java.Lang.String):

> RegularTextView.TextFormatted = Get().ProcessFormatted(new Java.Lang.String(Text)); // returns an expected processed text

So, this should have low priority (because there is a workaround to solve this) but it would be great if we can figure out why it's failing using c# string primitive.
Comment 1 Peter Collins 2017-09-13 20:14:41 UTC
I'm going to tentatively add this to the d15-4 milestone for tracking as it was encountered while working on new API 26 sample porting, and API 26 support will be shipping in d15-4.
Comment 2 Jonathan Pryor 2017-09-13 21:36:39 UTC
It's a `generator` bug. The problem is due to variable sharing.

Here is what `generator` produces for `EmojiCompat.Process(string)`:

> public string Process (string charSequence)
> {
> 	String @string = (charSequence == null) ? null : new String (charSequence);
> 	ICharSequence charSequence2 = this.ProcessFormatted (@string);
> 	if (@string != null) {
> 		@string.Dispose ();
> 	}
> 	if (charSequence2 != null) {
> 		return charSequence2.ToString ();
> 	}
> 	return null;
> }

(as per disassembly via Visual Studio for Mac)

If I change `InitCallbackImpl.OnInitialized()` to contain:

> Console.WriteLine($"# jonp: Text='{Text}'; Processed: {Get().Process(Text)}");
> var c = new Java.Lang.String (Text);
> Console.WriteLine($"# jonp: c='{c}'");
> var r = Get().ProcessFormatted(c);
> Console.WriteLine($"# jonp: r='{r}' r? {r != null}");
> Console.WriteLine($"# jonp: r == c? {ReferenceEquals(r, c)} r.H==c.H? {r.Handle == c.Handle}");

We get the `adb logcat` output:

> # jonp: Text='Regular TextView <U+1F469><U+200D><U+1F4BB> <U+1F469><U+200D><U+1F3A4>'; Processed: 
> # jonp: c='Regular TextView <U+1F469><U+200D><U+1F4BB> <U+1F469><U+200D><U+1F3A4>'
> # jonp: r='Regular TextView <U+1F469><U+200D><U+1F4BB> <U+1F469><U+200D><U+1F3A4>' r? True
> # jonp: r == c? True r.H==c.H? True

What we see from the above is that `EmojoCompat.ProcessFormatted(ICharSequence)` is returning the parameter unchanged. In this case, it's analogous to:

  public ICharSequence ProcessFormated(ICharSequence value) {
    return value;
  }

The input parameter instance *is* the return value.

Now, look again at the `Process(string)` overload. The "marshaled parameter" in `@string` is `Dispose()`d *before* the return value `charSequence2` is read. As such, we're disposing of the return value we want to use!

Rephrased, what we're effectively doing is this:

> var c = new Java.Lang.String (Text);
> var r = Get().ProcessFormatted(c);
> c.Dispose();
> var s = r.ToString();
> Console.WriteLine($"# jonp: s='{s}'");

`s` is the empty string, which is what we observe.
Comment 3 Jonathan Pryor 2017-09-13 21:42:47 UTC
`generator` fix: read the return value *before* disposing parameters. Instead of:

> 	String @string = (charSequence == null) ? null : new String (charSequence);
> 	ICharSequence charSequence2 = this.ProcessFormatted (@string);
> 	if (@string != null) {
> 		@string.Dispose ();
> 	}
> 	if (charSequence2 != null) {
> 		return charSequence2.ToString ();
> 	}
> 	return null;

We should instead do:

> 	String @string = (charSequence == null) ? null : new String (charSequence);
> 	ICharSequence charSequence2 = this.ProcessFormatted (@string);
> 	string __ret = null;
> 	if (charSequence2 != null) {
> 		__ret = charSequence2.ToString ();
> 	}
> 	@string?.Dispose();
> 	charSequence2?.Dispose();
> 	return __ret;
Comment 4 Jon Douglas [MSFT] 2017-09-14 15:55:04 UTC
I am not able to reproduce this issue on 15.4 preview 2. (Xamarin.Android 7.5.0.15)

I noticed the sample already included the workaround and thus I attempted using the method which was said to fail. i.e. EmojiCompat.Get().Process method. However trying this in both the InitCallback and also within the main OnCreate, I am seeing Emojis properly appearing even after reinstalling packages between deploys.

>    public override void OnInitialized()
>    {
>      RegularTextView.Text = EmojiCompat.Get().Process(Text);
>    }

https://i.imgur.com/UatWsxe.png

It seems JonP is onto something. However since I cannot confirm this bug, I will leave it in NEW status for the time being.
Comment 5 Jon Douglas [MSFT] 2017-09-14 16:02:01 UTC
Secondly, I had tested on a plethora of API levels as I first thought this could be an issue with whether it was using VectorDrawableCompat or VectorDrawable:

> For API 24 and above, this class is delegating to the framework's VectorDrawable. For older API version, this class lets you create a drawable based on an XML vector graphic.

However this seems to work on API 19->26 with the failing code per the original post.
Comment 6 Peter Collins 2017-09-14 18:57:39 UTC
Very interesting indeed... 

When using the original code snippet that the issue was reported against:

> RegularTextView.Text = EmojiCompat.Get().Process(Text);
I am still able to reproduce this when running on a Pixel XL w/ 8.0.0, as seen below:
https://screencast.com/t/3MfMfZBy

If I change the minimum deployment target and run on a Nexus 5 w/ 6.0 the string returned by EmojiCompat.Get().Process isn't empty, however the emojis which are displayed do differ from the Java version:
https://screencast.com/t/6xfNoPBuVqnu


Environment:
https://gist.github.com/pjcollins/711d5d149ba1d08a9255f26cbe279477
Comment 7 Jonathan Pryor 2017-09-14 20:33:12 UTC
PR: https://github.com/xamarin/java.interop/pull/185
Comment 8 Jonathan Pryor 2017-09-14 20:34:33 UTC
> If I change the minimum deployment target and run on a Nexus 5 w/ 6.0 the string 
> returned by EmojiCompat.Get().Process isn't empty, however the emojis which are 
> displayed do differ from the Java version:

The problem only happens when a parameter value is returned. In this case, a *different* value is returned, so disposing of the parameter intermediary doesn't invalidate the returned string, which is why it works there.