Bug 36556 - [Regression] XslCompiledTransform ignores disable-output-escaping attribute
Summary: [Regression] XslCompiledTransform ignores disable-output-escaping attribute
Status: NEW
Alias: None
Product: iOS
Classification: Xamarin
Component: BCL Class Libraries (show other bugs)
Version: 7.9.2.x
Hardware: Macintosh Mac OS
: Normal enhancement
Target Milestone: Untriaged
Assignee: Alexander Köplinger [MSFT]
URL:
Depends on:
Blocks:
 
Reported: 2015-12-03 19:46 UTC by Jon Goldberger [MSFT]
Modified: 2016-07-26 15:02 UTC (History)
7 users (show)

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


Attachments
Test Project (10.78 KB, application/zip)
2015-12-03 19:48 UTC, Jon Goldberger [MSFT]
Details
Console Repro (1.54 KB, application/zip)
2015-12-16 01:26 UTC, Jonathan Pryor
Details
Servelec-Example.zip (25.11 KB, application/zip)
2015-12-16 12:15 UTC, David Ingham
Details

Description Jon Goldberger [MSFT] 2015-12-03 19:46:43 UTC
## Description

It appears if we build our application with the latest version of Xamarin Studio the resulting application seems to be ignoring disable-output-escaping="yes" when applying an xml transformation.

Senario.
We have an dynamic form entry process which transforms some xml to an html for with javascript etc. Part of that is some dynamic conditions and when it's rendering the javascript it gets malformed e.g.

If( condition 1 <xsl:value-of select="&amp;&amp'" disable-output-escaping="yes"/> condition2) {

It's being output as:

If( condition1 &amp;&amp; condition2) { with the latest version of Xamarin studio.

If( condition1 && condition2) { with the other version of Xamarin Studio on our build server.

## Steps to reproduce

1. Open the attached test project. 

2. Deploy app to device or simulator.

3. Tap the "Apply XML Transform button on the bottom.

Expected result: Output in the Results field will be:
> <?xml version="1.0" encoding="utf-16"?><test>This is a test.&&</test>

Actual result: Output in Results field is:
> <?xml version="1.0" encoding="utf-16"?><test>This is a test.&amp;&amp;</test>

## Regression status.

This started occurring with Cycle 6. 

I tested with XI 8.10.526 (XCode 6.3.1) and got the expected output. So with only changing the version of XI (and then XCode as 8.10.526 is not compatible with XCode 7.1.1) 

Working and non-working environments below.

## Test environment (full version information)

Non-working environment:

=== Xamarin Studio ===

Version 5.10.1 (build 3)
Installation UUID: 964c531b-d928-456b-a9ae-e1f82266b360
Runtime:
	Mono 4.2.1 (explicit/6dd2d0d)
	GTK+ 2.24.23 (Raleigh theme)

	Package version: 402010102

=== Xamarin.Profiler ===

Version: 0.22.0.0
Location: /Applications/Xamarin Profiler.app/Contents/MacOS/Xamarin Profiler

=== Xamarin.Android ===

Version: 6.0.0.34 (Business Edition)
Android SDK: /Users/apple/Library/Developer/Xamarin/android-sdk-mac_x86
	Supported Android versions:
		4.0.3 (API level 15)
		4.1   (API level 16)
		4.2   (API level 17)
		4.3   (API level 18)
		4.4   (API level 19)
		5.0   (API level 21)
		5.1   (API level 22)
		6.0   (API level 23)

SDK Tools Version: 24.4.1
SDK Platform Tools Version: 23.0.1
SDK Build Tools Version: 23.0.2

Java SDK: /usr
java version "1.7.0_79"
Java(TM) SE Runtime Environment (build 1.7.0_79-b15)
Java HotSpot(TM) 64-Bit Server VM (build 24.79-b02, mixed mode)

=== Xamarin Android Player ===

Version: 0.6.5
Location: /Applications/Xamarin Android Player.app

=== Apple Developer Tools ===

Xcode 7.1.1 (9081)
Build 7B1005

=== Xamarin.iOS ===

Version: 9.2.1.54 (Business Edition)
Hash: eb4c1ef
Branch: master
Build date: 2015-12-01 02:12:30-0500

=== Xamarin.Mac ===

Version: 2.4.0.109 (Business Edition)

=== Build Information ===

Release ID: 510010003
Git revision: f2021a209d66d49cbc0649a6d968b29040e57807
Build date: 2015-12-01 10:43:40-05
Xamarin addins: dfd4f5103e8951edbc8ac24480b53b53c55e04ff
Build lane: monodevelop-lion-cycle6-baseline

=== Operating System ===

Mac OS X 10.11.1
Darwin Jons-iMac.local 15.0.0 Darwin Kernel Version 15.0.0
    Sat Sep 19 15:53:46 PDT 2015
    root:xnu-3247.10.11~1/RELEASE_X86_64 x86_64


Working environment:

As above but with:
Xamarin.iOS Version: 8.10.526
XCode version 6.3.1
Comment 1 Jon Goldberger [MSFT] 2015-12-03 19:48:05 UTC
Created attachment 14107 [details]
Test Project
Comment 3 Sebastien Pouliot 2015-12-03 20:23:50 UTC
This is likely due to the newer Mono (4.2) version being used in C6, which contains a lot of MS RS code changes, versus C5's Mono 4.0.
Comment 4 David Ingham 2015-12-09 10:33:24 UTC
Any update?
Comment 5 Rodrigo Kumpera 2015-12-09 17:48:47 UTC
Alexander, could you look at this one?

CC'ing Atsushi as he is our XML Jedi master and can help you figure out this issue.
Comment 6 Atsushi Eno 2015-12-09 18:39:40 UTC
I don't think we can fix that. Because 1) our XslTransform was too nice than .NET that it could handle disable-output-escaping even with XmlWriter output (since "disabling escaping is simply invalid and should not be doable", .NET ignored that specification in .xsl), and 2) we just dispatch to XslTransform on mobiles because we cannot get compiled XSL transform working (dynamic codegen).

There should be some combination of arguments that supports disable-output-escaping (like TextWriter or Stream I assume) and that should be used.
Comment 7 Atsushi Eno 2015-12-09 18:45:47 UTC
This is how I could manage to get "disable-output-escaping" on console project, based on the repro project.

using System;
using System.Xml;
using System.Xml.XPath;
using System.Xml.Xsl;
using System.IO;
using System.Text;

public class Test
{
	public static void Main (string [] args)
	{
		var xslt = new XslTransform();

		using (var stringReader = new StringReader(File.ReadAllText(args [0])))
		{
			using(var xmlReader = XmlReader.Create(stringReader))
			{
				xslt.Load(xmlReader);
			}
		}

		using (var stringReader = new StringReader(File.ReadAllText(args[1])))
		{
			using (var xmlreader = XmlReader.Create(stringReader))
			{
				var nav = new XPathDocument (xmlreader).CreateNavigator ();
				var sw = new StringWriter ();
				xslt.Transform(nav, null, sw);
				Console.WriteLine (sw);
			}
		}
	}
}
Comment 8 David Ingham 2015-12-10 12:30:47 UTC
Really?? Can't say I'm over the moon you're directing to me to use obsolete deprecated API. 

Warning CS0618: `System.Xml.Xsl.XslTransform' is obsolete: `This class has been deprecated. Please use System.Xml.Xsl.XslCompiledTransform instead. http://go.microsoft.com/fwlink/?linkid=14202'

If you can't make API that behaves the same as .net it shouldn't be in there surely.

I've tested the same code I supplied using an XslCompileTransform in pure .net in Visual Studio and it doesn't ignore the disable-output-escaping.

I've tested your code using an XslTransform in pure .net anyway and it doesn't ignore disable-output-escaping.

I know in my example it's spitting an xml document and thats invalid xml to have <element>text&&</element>.... it was just to quickly show the issue... in our real world example we're spitting out HTML and Javascript. which doesn't like if(condition1 &amp;&amp; condition2) { } since it's not disabling the output escaping anymore.

I'll be referring this to our account manager as I'm not very happy with this response.

From my perspective:
* This works in .net
* This used to work in a previous version of your Xamarin Studio product.
* This doesn't work in the current version of your Xamarin Studio product.
* Please fix your Xamarin Studio product.
Comment 9 Atsushi Eno 2015-12-10 14:06:54 UTC
Going back to old implementation is no-go. The new .NET behavior is much better than old implementation in many many ways. Our old XslCompiledTransform is not really a compiler (dynamic code) based anyways.

The required changes is very minimum and basic .NET programmers can easily fix that.

At the previous comment I logically explained why it is impossible to use XslCompiledTransform. Even Microsoft cannot do that.

You would like to show these comments to your manager. I believe my comments are from very fair poin of view.

And it has nothing to do with Xamarin Studio.
Comment 10 Jonathan Pryor 2015-12-16 01:26:56 UTC
Created attachment 14310 [details]
Console Repro
Comment 11 Jonathan Pryor 2015-12-16 01:32:13 UTC
Comment #8 states:

> * This works in .net

Could you please attach your repro? Ideally a console app?

I must admit that I'm finding this thread confusing. Cycle 6 [0] uses Mono 4.2, which in turn has replaced (most off? allof ?) the System.Xml implementation with the microsoft/referencesource implementation, which (ideally) is equivalent/identical to what's in .NET. I thus don't understand how Cycle 6 behavior could differ from .NET

Which brings me to my repro: Attachment #14310 [details], which attempts to extract the XSLT and XML from Attachment #14107 [details] into a console app:

  $ curl -o a.zip https://bugzilla.xamarin.com/attachment.cgi?id=14310
  $ unzip a.zip
  $ cd bxc-36556
  $ make run
  mcs /out:app.exe app.cs
  mono app.exe transform.xslt in.xml
  <?xml version="1.0" encoding="utf-16"?>
  <test>This is a test.&&</test>

Note that the output is what is requested in Comment #0.

  <test>This is a test.&&</test>

Further note that it's using the XSLT specified in Comment #0, *and* is using XslCompiledTransform (so no warnings when compiling):

  <xsl:value-of select="&amp;&amp'" disable-output-escaping="yes"/>

[0]: https://releases.xamarin.com/stable-release-cycle-6-final/
Comment 12 David Ingham 2015-12-16 12:15:12 UTC
Created attachment 14314 [details]
Servelec-Example.zip
Comment 13 David Ingham 2015-12-16 12:16:50 UTC
Using a string writer does some to obey the disable-output-escaping but using an xmlwriter doesn't in xamarin studio.
The attached example shows this using .Net / Console and then a Xamarin Studio Project.

1) curl -o servelec-example.zip https://bugzilla.xamarin.com/attachment.cgi?id=14314
2) unzip servelec-example.zip
3) cd console 
4) make run

Ouptut:
mcs /out:app.exe app.cs
mono app.exe transform.xslt in.xml
<?xml version="1.0" encoding="utf-16"?><test>This is a test.&&</test>

5) cd ..
6) cd xamarin-studio

Version Info & Screen shot of result can be found here.

7) cd xsltbug
8) open xsltbug.sln
9) Select Debug | iPhone
10) Run

Output:
<?xml version="1.0" encoding="utf-16"?><test>This is a test.&amp;&amp;</test>
Comment 14 Jonathan Pryor 2015-12-17 20:47:48 UTC
Upon closer analysis, the Xamarin.iOS code in Attachment #14314 [details] (Comment #12) is NOT like the code in Attachment #14310 [details] and Comment #7.

The failing code from Comment #12 is:

  var stringBuilder = new StringBuilder();
  using (var xmlWriter = XmlWriter.Create(stringBuilder))
  {
    xslt.Transform(xmlreader, xmlWriter);
  }
  return stringBuilder.ToString();

Meanwhile, the other versions do:

  var nav = new XPathDocument(xmlreader).CreateNavigator();
  var sw = new StringWriter ();
  xslt.Transform (nav, null, sw);
  return sw.ToString ();

(or some variation thereof).

The former generates the undesired `&amp;&amp;` output. the latter generates the desired `&&` output, *even in* Xamarin.iOS 9.4.0.0.

Note that different overloads of XslCompiledTransform.Transform() are invoked:

"Bad": https://msdn.microsoft.com/en-us/library/ms163433(v=vs.110).aspx

"Good": https://msdn.microsoft.com/en-us/library/ms163435(v=vs.110).aspx

I don't know why this is the case; using the Transform(XmlReader, XmlWriter) overload in a console app works as expected.
Comment 15 Jonathan Pryor 2015-12-17 20:50:23 UTC
@Eno: Xamarin.Android behaves the same as Xamarin.iOS here, in that using XslCompiledTransform.Transform(XmlReader, XmlWriter) produces different output than using XslCompiledTransform.Transform(IXPathNavigable, XsltArgumentList, TextWriter).

More importantly, to my mind, is that Xamarin.iOS and Xamarin.Android behavior differs from desktop mono 4.2.1 (explicit/6dd2d0d), which makes no sense to me.

I suspect that there's something "missing" in the MOBILE profile which is present in the full/desktop profile which is introducing this discrepancy.

Could you please investigate?
Comment 16 Marek Safar 2015-12-18 08:48:15 UTC
XI and XA behave differently to full mono because they are running different code. Only desktop mono uses full reference source System.XML implementation. Mobile profiles still use bits from old mono code which still need to be maintained.

See e.g. https://github.com/mono/mono/blob/master/mcs/class/System.XML/System.Xml.Xsl/XslCompiledTransform_Mobile.cs
Comment 17 David Ingham 2015-12-18 09:52:46 UTC
We can't just switch to a string writer as this then breaks the underlying javascript as the output is formatted differently to that produced with the xmlwriter. We can't just change the javascript to handle it as this would mean rounds of testing and upstream / downstream of a work around and deployment to our customers, customer testing to accept those changes as they would impact our main stream web based product.
Comment 18 Jonathan Pryor 2016-01-05 20:51:03 UTC
This is...hilarious? Humorous? Terrifying? Yes?

Let's see if I can summarize this thread properly, because this thread is *really* confusing me.

Restating things:

We have two types: the [Obsolete] XslTransform, and its replacement, XslCompiledTransform.

The Microsoft XslCompiledTransform implementation requires System.Reflection.Emit, and thus is NOT used in the MOBILE profile used by Xamarin.Android and Xamarin.iOS (because iOS doesn't support runtime code execution; Android *could*, but doing so would introduce a behavioral mismatch between iOS & Android, which might be fine...but might be confusing).

Consequently, in the MOBILE profile XslCompiledTransform "just" delegates to XslTransform (Comment #16). We will henceforth *ignore* XslCompiledTransform, because on MOBILE it IS XslTransform.

XslTransform provides Transform() methods which are overloaded to take XmlWriter, TextWriter, or Stream:

https://msdn.microsoft.com/en-us/library/2adza3sz(v=vs.110).aspx
https://msdn.microsoft.com/en-us/library/ms163486(v=vs.110).aspx

The problem is that using the XmlWriter overload emits the "wrong" XML, as the disable-output-escaping attribute is ignored.

Hopefully this correctly summarizes the thread so far?

Which brings us to the source of this bug: the XslTransform.Transform(IXPathNavigable, XmlWriter) method is *documented* as *ignoring* disable-output-escaping!

https://msdn.microsoft.com/en-us/library/ms163486(v=vs.110).aspx
> The xsl:output element is not supported when outputting to an XmlWriter
> (xsl:output is ignored). See Outputs from an XslTransform for more information.

This links to:
https://msdn.microsoft.com/en-us/library/s49f0f17(v=vs.110).aspx
> The disable-output-escaping attribute is ignored when transforming to an
> XmlReader or XmlWriter object and has no effect on special characters.

The behavior is thus *documented*, *expected* behavior for the XslTransform.Transform(..., XmlWriter) method overloads. We thus cannot fix XslTransform; there's nothing to fix! (Unless we have a time machine...)

**However**, this *does* mean that XslCompiledTransform.Transform(..., XmlWriter) behaves inconsistently with desktop Mono/.NET, as the *real* XslCompiledTransform(..., XmlWriter) methods *don't* ignore disable-output-escaping. (Meaning on desktop Mono/.NET, XslTransform and XslCompiledTransform behavior are not consistent with each other. Fun!)
Comment 19 Jonathan Pryor 2016-01-05 20:53:22 UTC
A *plausible* fix then would be to add an internal XslTransform.TransformWithDisabledEscaping(..., XmlWriter) method and use that in the MOBILE XslCompiledTransform implementation.

Problem: I have no idea if doing such a thing is possible or how long it would take to implement; I don't currently understand the System.Xml implementation.
Comment 20 Jonathan Pryor 2016-01-05 21:05:46 UTC
@Ingham: From Comment #17:

> We can't just switch to a string writer as this then breaks the underlying javascript as the output is formatted differently to that produced with the xmlwriter.
> ...

This doesn't make any sense to me (and/or is going over my head).

Sure, the output is formatted differently, *but it's still XML*. At least for the tests on this bug, the difference between the TextWriter and XmlWriter overloads is an inserted newline character after the XML declaration.

It boggles my mind that this change would break anything, and if that change *does* break anything...

I don't know how to complete that thought.
Comment 21 Atsushi Eno 2016-01-06 05:19:36 UTC
Comment #18 - is correct.

The right "fix" is to mark XslCompiledTransform as [Obsolete ("You cannot really use it because it depends on dynamic code generation. You can only use XslTransform, and it just dispatches to XslTransform either way.")].
Then It is then super clear to everyone. I recommend to do this and close this bug as FEATURE.

Comment #16 - is almost correct - our System.Xml, namely XslTransform (old implementation) is totally referencesource based, thus it behaves differently than before.

To make sure, it is impossible to bring back only old XslTransform implementation because XSLT highly depends on XPath.

Comment #19 doesn't sound realistic to me. If we bring a lot of changes that's not Microsoft's stable referencesource that people want anymore.
Comment 22 Jonathan Pryor 2016-01-06 14:56:26 UTC
@Eno: The question in Comment #19 was how many changes would be needed to do it, assuming it's even *possible* (and it might not be)? Perhaps it's implementable entirely in partial classes, involving ~no changes to referencesource (except maybe adding `partial class`?). Perhaps not; I have no idea. :-(

It might be nice to at least take a stab at implementing Comment #19 if only to see what issues arise.

I have no idea what timeframe this could be done in.
Comment 23 David Ingham 2016-01-11 10:55:22 UTC
@Pryor - Comment: 18
>This is...hilarious? Humorous? Terrifying? Yes?
Not really finding it hilarious or humorous. Terrifying, annoying, frustrating..... yes.

>This links to:
>https://msdn.microsoft.com/en-us/library/s49f0f17(v=vs.110).aspx
>> The disable-output-escaping attribute is ignored when transforming to an
>> XmlReader or XmlWriter object and has no effect on special characters.

When then whole sentence is quoted:
"The <xsl:text disable-output-escaping> tag is used to indicate whether or not special characters need to be escaped into an XML form (for example, using <&lt> in place of the "<" symbol) or left in the present condition. The disable-output-escaping attribute is ignored when transforming to an XmlReader or XmlWriter object and has no effect on special characters."

This cites one tag <xsl:text> where the attribute is ignored. We're using <xsl:value-of/>. When I've done the transform with an XMLWriter using <xsl:value-of select... disable-output-escaping="yes"> respected and worked as is desired.



@Pryor - Comment: 20

>> This doesn't make any sense to me (and/or is going over my head).
>> Sure, the output is formatted differently, *but it's still XML*

As I've said, our application creates a dynamic html form based the form definition which is in XML and transformed to produced the HTML.

If we use an XmlWriter we get an output like:
<html><head><script>function foo(){...}</script></head><body>...</body></html>

If you use a StringWriter you get:
<html>
    <head>
        <script>
            function foo()
            {
                ...
            }
        </script>
    </head>
    <body>
        ...
    </body>
</html>

The StringWriter output then breaks the form as javascript calls to previous siblings etc now return empty text (the new line / tb chars) instead of the desired element. This is just one quirk i found with a casual prod of our form. If we went to to string writer we'd have to re-validate all the forms logic again which needs effort from our testing team as forms is a very complex area of our functionality, its not just simple data capture. Also people working on the web front end version would be coding against essentially different html so the chance of something getting through testing in web and then breaking our iPad version is greatly increased.

So we can't just goto a string writer.



@Eno - Comment 21
> The right "fix" is to mark XslCompiledTransform as 
> [Obsolete ("You cannot really use it because it depends 
> on dynamic code generation. You can only use  XslTransform
> , and it just dispatches to XslTransform either way.")].
>
>Then It is then super clear to everyone. I recommend to do this and close this bug as FEATURE.

I don't want this closed until it's properly resolved. We've been skuppered for production use of the Xamarin Studio since this bug was introduced. This is a big. Everything worked fine in native .net etc. The only area with a problem has been on the Xamarin.iOS implementation since the version I moved to at the start of this bug. I don't care if it's doing a compiled transform under hood or not. I just want tht xml that is outputted to be the same as it was before these changes. The same as .net etc. 

Also XlsTransform as depricted and by microsoft to point people to compiled transform if you depricate compiled  transform and make everyone implement depricated code again how is this good practice. Like i said don't care what Xamarin.iOS is doing under the hood for XslCompiledTransform i just want the output to be the same as regular .net.
Comment 24 David Ingham 2016-01-12 09:22:23 UTC
After some further poking I see that the MSDN page does infact apply to xsl:value-of as well when using xslTransform and an XmlWriter and apologise for my misunderstanding.

This doesn't change the fact that the Xamarin.iOS XslCompileTransform (regardless if it just used you implementation of XslTransform under the hood and you've now change the XslTransform implmentation) used to output the correctly for XslCompiledTransform and now doesn't. I object to this just been marked as an enhancement to be done as a when. This is a BUG as I've stated before numerous times and puts us in a awkward place.
Comment 25 Atsushi Eno 2016-01-12 10:41:37 UTC
From https://bugzilla.xamarin.com/page.cgi?id=fields.html#importance (This is linked right next to the items on this bugzilla page):

Importance
The importance of a bug is described as the combination of its Priority and Severity.

Priority
Engineers prioritize their bugs using this field.

Severity
How severe the bug is, or whether it's an enhancement.

All of "How you can workaround the issue", "Why this happens technically" and "How problamtic if we made changes" are already explained.
Comment 26 Marek Safar 2016-01-14 19:02:17 UTC
As described before this is a regression but we don't have easy way to fix it. This is not the only issue we have, there are few tests failing too due to incompatibilities.

Possible options

#1 Revert to Mono 4.0
That's not really doable because XslCompiledTransform is large code with many dependencies where some of the types are shared for different xml api. So we'd ended up with 2 system.xml dlls

#2 Implement interpreter XslCompiledTransform generated AST.
That's possible but quite large effort considering it uses many SRE opcodes and generates many methods.

#3 Add hacks to reference source XslTransform
One of the easier options but still requires large code changes as XslTransform supports much less than XslCompiledTransform

#4 Get linker involved
We could run XslCompiledTransform during/before linking and only include generated state machine dll. That way produced code will be much smaller and could cover most of cases because XSLT are usually static anyway. There is even XSLTC.exe utility which does something similar.
Comment 27 Miguel de Icaza [MSFT] 2016-01-18 21:03:41 UTC
Hello,

We are exploring the avenues of how to address this problem.
Comment 28 David Ingham 2016-01-19 08:29:03 UTC
Thank you, that is all I wanted rather than being told go away and re-write your app to work around these moving goal posts while it went on the back burner.
Comment 29 David Ingham 2016-07-26 15:02:27 UTC
Any movement on this  6 months down the line? I've just gone to Xamarin Studio 6 and the previously supplied word around of changing the symbolic link in:
/Library/Frameworks/Xamarin.iOS.framework/Versions/Current/lib/mono/Xamarin.iOS

to point to 8.10.5.26 versions of system.xml.dll where it worked as expected etc has stopped working.

Note You need to log in before you can comment on or make changes to this bug.