fix: Encode XML files as UTF-8 to fix compilation of resources

[Taknok]

The android implementation of the transformer encodes the emoji in a form called "surrogate", but aapt and apktool do not understand the surrogate form. Thus, detect surrogate and recompute the traditional code point value before writing it to file allowing aapt and apktool to rebuild the app successfully.

It should fix some cases (mainly for google app) of ReVanced/revanced-manager#2142

[LisoUseInAIKyrios]

For reference, this different solution appears to fix the same issue by exporting to a utf16 string then recoding to utf8.

At first glance I'm not sure if it's better or worse than the solution in this PR.

[Taknok]

I tried the modifications in the abandoned PR without success for the rebuild of the google phone app.

Should I rebase my branch on dev ?

[LisoUseInAIKyrios]

Rebasing is best since it's merging to dev.

[oSumAtrIX]

I dislike band-aid solutions. For a couple of painful days I am experimenting a proper solution such as using a different transformer implementation like Saxon. Hopefully a proper solution can be found. If not, I'd prefer to outsource this fix-up solution as a wrapped document() API in patches repo

[Taknok]

I agree, but the cleanest would be to fix the android transformer (to keep unicode point) or the aapt code (to handle surrogate form). I was not sure about introducing a new lib in the source code. Thus, I opted for the less impacting solution.

The wrapped solution or a argument for document() could be possible to. I could update the PR for this.

[Taknok]

The core of the issue lies in the Android SDK based on OpenJDK, which in turn include Apache Xalan code. Java represents characters using UTF-16 (source). As a result, high Unicode code points are split into surrogate pairs (two char values). When parsed by Xalan's TreeWalker (link), these pairs are processed as two separate characters.

Subsequently, ToXMLStream or ToStream fails to recognize the two utf16 chars as a single Unicode character and encodes each part separately, producing an incorrect surrogate representation. This is a known issue, tracked by the following tickets:

• https://issues.apache.org/jira/browse/XALANJ-2560
• https://issues.apache.org/jira/browse/XALANJ-2419

The ToHTMLStream has been fixed (PR #163), but no release has been published since 2023, so the fix is not available in official SDKs like Android or OpenJDK.

Conclusion: A fix for the Xalan transformer is unlikely to be available in the near future.

I also explored using the Saxon-HE transformer. Unfortunately, Saxon is not designed to support Android. During transformer creation, it depends on javax.xml.transform.stax and javax.xml.stream—two packages that Google chose not to include in Android’s core Java libraries. You can verify this in the list of supported packages: https://developer.android.com/reference/packages.html

To use Saxon, we would need to port those two packages to Android, which is likely beyond the scope of this repository.

That leaves us with three options:

1. Adopt a third-party XSLT transformer that supports Android. But which one ?
2. Use the regex-based approach proposed in this PR. On 10 runs I have an average of 1m21s for patching the google phone app
3. Convert the full string to UTF-16 and back to UTF-8, letting the cast merge surrogate bytes correctly On 10 runs I have an average of 1m18s for patching the google phone app

Regex based : Taknok@c20e804
UTF casting : Taknok@ba57bcb

@oSumAtrIX What would be the best ? the 3rd option with an optional parameter for the context.document(resource, enforce_utf8=true).use... ?
By default the enforce would be false

[oSumAtrIX]

I was thinking of a more efficient solution which is to hook the output stream and modify it. This avoids writing to string, modifying it, then writing it to the file:

ChatGPT gave this as a starter:

import java.io.OutputStream
import java.io.Writer
import java.io.OutputStreamWriter

class UnicodeCorrectingWriter(outputStream: OutputStream, charset: String = "UTF-8") : Writer() {
    private val writer = OutputStreamWriter(outputStream, charset)
    private var pendingHighSurrogate: Char? = null

    override fun write(cbuf: CharArray, off: Int, len: Int) {
        for (i in off until off + len) {
            val ch = cbuf[i]
            if (Character.isHighSurrogate(ch)) {
                pendingHighSurrogate = ch
            } else if (Character.isLowSurrogate(ch) && pendingHighSurrogate != null) {
                val codePoint = Character.toCodePoint(pendingHighSurrogate!!, ch)
                writer.write(String(Character.toChars(codePoint)))
                pendingHighSurrogate = null
            } else {
                if (pendingHighSurrogate != null) {
                    // Write invalid pending high surrogate as-is
                    writer.write(pendingHighSurrogate!!.toInt())
                    pendingHighSurrogate = null
                }
                writer.write(ch.toInt())
            }
        }
    }

    override fun flush() {
        if (pendingHighSurrogate != null) {
            writer.write(pendingHighSurrogate!!.toInt())
            pendingHighSurrogate = null
        }
        writer.flush()
    }

    override fun close() {
        flush()
        writer.close()
    }
}

I haven't tested it but maybe this works. Would merge if this approach can be used.

[Taknok]

I will look into it, thank for the hint

[Taknok]

After quick look, the code never enter the highsurrogate, thus it never correctly encode the emoji.

if (Character.isHighSurrogate(ch)) {
  pendingHighSurrogate = ch

Furthermore it takes more than 4min to reach the crash when repacking the res. I will continue to look into it to make it works, but it seems to not be the best performance strategy.

[Taknok]

So unfortunately, the chatGPT method does not work as is, as the ToStream serializer write char by char to the stream when we reach this block : https://android.googlesource.com/platform/external/apache-xml/+/650a6cfd4d6b2d38b88ada03694ae19cc448d07b/src/main/java/org/apache/xml/serializer/ToStream.java#1609
The writer has to record the state of the potential surrogate and process the logic (as done in the xalan updated ToStream.java code).
The performance of this method is the lower among all. It tooks 1m42s average (when ~1m20s for the others). Due to the complexity of the writer, this method seems to be the less futur proof as it depends of the calls of ToStream.

My tracking branch: Taknok@ca2de8e

My personnal ranking of the techniques:

1. UTF casting Taknok@ba57bcb
2. regex Taknok@c20e804
3. writer Taknok@ca2de8e

Available on discord if needed.

[kitadai31]

1. UTF casting Taknok@ba57bcb

Wait, does setOutputProperty(OutputKeys.ENCODING, "UTF-16") work on Android Runtime!?

Yeah, I also confirmed with Android 6.0 and 15.
I didn’t know that at all...

Because it doesn't work on JRE.
On JRE, setOutputProperty(OutputKeys.ENCODING, "XXX") only has an effect when a Document instance is created on the memory.
If a Document is created from an existing XML file, it will have no effect.

References:
https://discord.com/channels/952946952348270622/953965039105232906/1333603549904375909
https://stackoverflow.com/questions/15592025/transformer-setoutputpropertyoutputkeys-encoding-utf-8-is-not-working
https://bugs.openjdk.org/browse/JDK-8227616

Then, you don't need to use StringWriter or convert to UTF-8.
AAPT2's XML parser understands encoding="UTF-16" and can compile UTF-16 XML files successfully.
So just output with UTF-16.

Adding just one line will fix the issue.

TransformerFactory.newInstance()
    .newTransformer()
    .apply { setOutputProperty(OutputKeys.ENCODING, "UTF-16") }  // Add like this?
    .transform(DOMSource(this), StreamResult(stream))

(a condition for checking AndroidRuntime could be added)

[oSumAtrIX]

If this is all and a conditional check is added, I'd merge it with a comment referencing the Android issue tracker

[Taknok]

There is not surrogate issue on JRE as the openjdk correctly implement the conversion. Thus the conditional check is still needed ?

I will try without the UTF8 , but I think y my tests it was not working without

[Taknok]

Here are the requested modifications. Tell me if you want some adjustments.

[kitadai31]

It worked!
I patched GPhotos on Manager with Patches 5.19.0 (the last patches version that photos compiling is not fixed) successfully.

[Taknok]

You can also use google phone app with this patch to check if issue is fixed : https://github.com/Taknok/revanced-patches

[kitadai31]

Move this to companion object so that you can avoid checking this every time

Also, System.getProperty("java.runtime.name") == "Android Runtime" is simpler

(This property has probably not been changed since the beginning, Even in Android 4.x, before ART was introduced, it was “Android Runtime”.)

[kitadai31]

Nope, outputting in UTF-16 caused problems when patching other apps
Converting to UTF-8 is probably necessary

Sorry for jumping to conclusions...

- Device Info
ReVanced Manager: 1.25.0-dev.1
Model: Pixel 3
Android version: 15
Supported architectures: arm64-v8a, armeabi-v7a, armeabi
Root permissions: Yes

- Patch Info
App: com.google.android.youtube v20.12.46 (Suggested: 20.12.46)
Patches version: v5.22.0
Patches added: Default
Patches removed: None
Default patch options changed: None

- Settings
Allow changing patch selection: false
Version compatibility check: true
Show universal patches: false
Patches source: revanced/revanced-patches

- Logs
Reading APK
Decoding app manifest
Loading patches
Deleting existing temporary files directory
Decoding resources
Initializing lookup maps
Executing patches
Applied 52 patches
GmsCore support failed: app.revanced.patcher.patch.PatchException: The patch "GmsCore support" depends on "ResourcePatch", which raised an exception:
app.revanced.patcher.patch.PatchException: Unexpected token (position:TEXT ���@1:8 in java.io.InputStreamReader@aec901d) 
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend$execute(Unknown Source:143)
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend$execute(Unknown Source:65)
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend(Unknown Source:190)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:2)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:1)
	at kotlinx.coroutines.flow.SafeFlow.collect(Unknown Source:102)
	at app.revanced.manager.flutter.MainActivity$runPatcher$1$patcherResult$1$1.invokeSuspend(Unknown Source:74)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(Unknown Source:11)
	at kotlinx.coroutines.DispatchedTask.run(Unknown Source:134)
	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(Unknown Source:91)
	at kotlinx.coroutines.JobKt.runBlocking$default(Unknown Source:104)
	at app.revanced.manager.flutter.MainActivity.runPatcher$lambda$34(Unknown Source:360)
	at app.revanced.manager.flutter.MainActivity.$r8$lambda$_NYn7P0Ss3RCL0-SqEFL-opMsQw(Unknown Source:0)
	at app.revanced.manager.flutter.MainActivity$$ExternalSyntheticLambda6.run(Unknown Source:18)
	at java.lang.Thread.run(Thread.java:1119)
Caused by: org.xml.sax.SAXParseException: Unexpected token (position:TEXT ���@1:8 in java.io.InputStreamReader@aec901d) 
	at org.apache.harmony.xml.parsers.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:147)
	at javax.xml.parsers.DocumentBuilder.parse(DocumentBuilder.java:107)
	at app.revanced.patcher.util.Document.<init>(SourceFile:2)
	at app.revanced.patcher.util.Document.<init>(SourceFile:4)
	at app.revanced.patcher.patch.ResourcePatchContext.document(SourceFile:2)
	at app.revanced.patches.shared.misc.gms.GmsCoreSupportPatchKt.gmsCoreSupportResourcePatch$lambda$35$lambda$34$addSpoofingMetadata(GmsCoreSupportPatch.kt:568)
	at app.revanced.patches.shared.misc.gms.GmsCoreSupportPatchKt.gmsCoreSupportResourcePatch$lambda$35$lambda$34(GmsCoreSupportPatch.kt:622)
	at app.revanced.patches.shared.misc.gms.GmsCoreSupportPatchKt.$r8$lambda$lRRewJm6n_a-02ysO3PqlF2CFs8(Unknown Source:0)
	at app.revanced.patches.shared.misc.gms.GmsCoreSupportPatchKt$$ExternalSyntheticLambda3.invoke(Unknown Source:15)
	at app.revanced.patcher.patch.Patch.execute(Unknown Source:7)
	at app.revanced.patcher.patch.ResourcePatch.execute$revanced_patcher(Unknown Source:9)
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend$execute(Unknown Source:124)
	... 14 more

	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend$execute(Unknown Source:113)
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend(Unknown Source:190)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:2)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:1)
	at kotlinx.coroutines.flow.SafeFlow.collect(Unknown Source:102)
	at app.revanced.manager.flutter.MainActivity$runPatcher$1$patcherResult$1$1.invokeSuspend(Unknown Source:74)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(Unknown Source:11)
	at kotlinx.coroutines.DispatchedTask.run(Unknown Source:134)
	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(Unknown Source:91)
	at kotlinx.coroutines.JobKt.runBlocking$default(Unknown Source:104)
	at app.revanced.manager.flutter.MainActivity.runPatcher$lambda$34(Unknown Source:360)
	at app.revanced.manager.flutter.MainActivity.$r8$lambda$_NYn7P0Ss3RCL0-SqEFL-opMsQw(Unknown Source:0)
	at app.revanced.manager.flutter.MainActivity$$ExternalSyntheticLambda6.run(Unknown Source:18)
	at java.lang.Thread.run(Thread.java:1119)
Change package name failed: app.revanced.patcher.patch.PatchException: The patch "Change package name" raised an exception: app.revanced.patcher.patch.PatchException: Unexpected token (position:TEXT ���@1:8 in java.io.InputStreamReader@9750f0a) 
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend(Unknown Source:320)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:2)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:1)
	at kotlinx.coroutines.flow.SafeFlow.collect(Unknown Source:102)
	at app.revanced.manager.flutter.MainActivity$runPatcher$1$patcherResult$1$1.invokeSuspend(Unknown Source:74)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(Unknown Source:11)
	at kotlinx.coroutines.DispatchedTask.run(Unknown Source:134)
	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(Unknown Source:91)
	at kotlinx.coroutines.JobKt.runBlocking$default(Unknown Source:104)
	at app.revanced.manager.flutter.MainActivity.runPatcher$lambda$34(Unknown Source:360)
	at app.revanced.manager.flutter.MainActivity.$r8$lambda$_NYn7P0Ss3RCL0-SqEFL-opMsQw(Unknown Source:0)
	at app.revanced.manager.flutter.MainActivity$$ExternalSyntheticLambda6.run(Unknown Source:18)
	at java.lang.Thread.run(Thread.java:1119)
Caused by: org.xml.sax.SAXParseException: Unexpected token (position:TEXT ���@1:8 in java.io.InputStreamReader@9750f0a) 
	at org.apache.harmony.xml.parsers.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:147)
	at javax.xml.parsers.DocumentBuilder.parse(DocumentBuilder.java:107)
	at app.revanced.patcher.util.Document.<init>(SourceFile:2)
	at app.revanced.patcher.util.Document.<init>(SourceFile:4)
	at app.revanced.patcher.patch.ResourcePatchContext.document(SourceFile:2)
	at app.revanced.patches.all.misc.packagename.ChangePackageNamePatchKt.changePackageNamePatch$lambda$9$lambda$8(ChangePackageNamePatch.kt:78)
	at app.revanced.patches.all.misc.packagename.ChangePackageNamePatchKt.$r8$lambda$6AkdPHArk5z26dp2iCI31m62ogY(Unknown Source:0)
	at app.revanced.patches.all.misc.packagename.ChangePackageNamePatchKt$$ExternalSyntheticLambda4.invoke(Unknown Source:6)
	at app.revanced.patcher.patch.Patch.finalize(Unknown Source:9)
	at app.revanced.patcher.patch.ResourcePatch.finalize$revanced_patcher(Unknown Source:9)
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend(Unknown Source:308)
	... 12 more

	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend(Unknown Source:382)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:2)
	at app.revanced.patcher.Patcher$invoke$1.invoke(SourceFile:1)
	at kotlinx.coroutines.flow.SafeFlow.collect(Unknown Source:102)
	at app.revanced.manager.flutter.MainActivity$runPatcher$1$patcherResult$1$1.invokeSuspend(Unknown Source:74)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(Unknown Source:11)
	at kotlinx.coroutines.DispatchedTask.run(Unknown Source:134)
	at kotlinx.coroutines.EventLoopImplBase.processNextEvent(Unknown Source:91)
	at kotlinx.coroutines.JobKt.runBlocking$default(Unknown Source:104)
	at app.revanced.manager.flutter.MainActivity.runPatcher$lambda$34(Unknown Source:360)
	at app.revanced.manager.flutter.MainActivity.$r8$lambda$_NYn7P0Ss3RCL0-SqEFL-opMsQw(Unknown Source:0)
	at app.revanced.manager.flutter.MainActivity$$ExternalSyntheticLambda6.run(Unknown Source:18)
	at java.lang.Thread.run(Thread.java:1119)
Caused by: app.revanced.patcher.patch.PatchException: Unexpected token (position:TEXT ���@1:8 in java.io.InputStreamReader@9750f0a) 
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend(Unknown Source:320)
	... 12 more
Caused by: org.xml.sax.SAXParseException: Unexpected token (position:TEXT ���@1:8 in java.io.InputStreamReader@9750f0a) 
	at org.apache.harmony.xml.parsers.DocumentBuilderImpl.parse(DocumentBuilderImpl.java:147)
	at javax.xml.parsers.DocumentBuilder.parse(DocumentBuilder.java:107)
	at app.revanced.patcher.util.Document.<init>(SourceFile:2)
	at app.revanced.patcher.util.Document.<init>(SourceFile:4)
	at app.revanced.patcher.patch.ResourcePatchContext.document(SourceFile:2)
	at app.revanced.patches.all.misc.packagename.ChangePackageNamePatchKt.changePackageNamePatch$lambda$9$lambda$8(ChangePackageNamePatch.kt:78)
	at app.revanced.patches.all.misc.packagename.ChangePackageNamePatchKt.$r8$lambda$6AkdPHArk5z26dp2iCI31m62ogY(Unknown Source:0)
	at app.revanced.patches.all.misc.packagename.ChangePackageNamePatchKt$$ExternalSyntheticLambda4.invoke(Unknown Source:6)
	at app.revanced.patcher.patch.Patch.finalize(Unknown Source:9)
	at app.revanced.patcher.patch.ResourcePatch.finalize$revanced_patcher(Unknown Source:9)
	at app.revanced.patcher.Patcher$invoke$1.invokeSuspend(Unknown Source:308)
	... 12 more
Compiling modified resources
/data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/apk/AndroidManifest.xml:1: error: not well-formed (invalid token).
An error occurred:
brut.androlib.exceptions.AndrolibException: brut.common.BrutException: could not exec (exit code = 1): [/data/app/~~uxzK18kQmnPAJcAbOvR0jw==/app.revanced.manager.flutter.development-yPDJNdvuT9xMrXHjOgBweQ==/lib/arm64/libaapt2.so, link, -o, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/patched/resources/resources.apk, --package-id, 127, --min-sdk-version, 26, --target-sdk-version, 35, --version-code, 1553610176, --version-name, 20.12.46, --no-auto-version, --no-version-vectors, --no-version-transitions, --no-resource-deduping, --allow-reserved-package-id, --warn-manifest-validation, -e, /data/user/0/app.revanced.manager.flutter.development/cache/APKTOOL9182088485613914028.tmp, -0, arsc, -I, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/1.apk, --manifest, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/apk/AndroidManifest.xml, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/apk/build/resources.zip]
	at app.revanced.patcher.patch.ResourcePatchContext.get(SourceFile:146)
	at app.revanced.patcher.Patcher.get(Unknown Source:18)
	at app.revanced.manager.flutter.MainActivity.runPatcher$lambda$34(Unknown Source:391)
	at app.revanced.manager.flutter.MainActivity.$r8$lambda$_NYn7P0Ss3RCL0-SqEFL-opMsQw(Unknown Source:0)
	at app.revanced.manager.flutter.MainActivity$$ExternalSyntheticLambda6.run(Unknown Source:18)
	at java.lang.Thread.run(Thread.java:1119)
Caused by: brut.common.BrutException: could not exec (exit code = 1): [/data/app/~~uxzK18kQmnPAJcAbOvR0jw==/app.revanced.manager.flutter.development-yPDJNdvuT9xMrXHjOgBweQ==/lib/arm64/libaapt2.so, link, -o, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/patched/resources/resources.apk, --package-id, 127, --min-sdk-version, 26, --target-sdk-version, 35, --version-code, 1553610176, --version-name, 20.12.46, --no-auto-version, --no-version-vectors, --no-version-transitions, --no-resource-deduping, --allow-reserved-package-id, --warn-manifest-validation, -e, /data/user/0/app.revanced.manager.flutter.development/cache/APKTOOL9182088485613914028.tmp, -0, arsc, -I, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/1.apk, --manifest, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/apk/AndroidManifest.xml, /data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/revanced-temporary-files/apk/build/resources.zip]
	at brut.util.OS.exec(Unknown Source:72)
	at app.revanced.patcher.patch.ResourcePatchContext.get(SourceFile:142)
	... 5 more
Something went wrong:
PathNotFoundException: Cannot copy file to '/data/user/0/app.revanced.manager.flutter.development/files/lastPatchedApp.apk', path = '/data/user/0/app.revanced.manager.flutter.development/files/patcher/tmp-KGGWPD/out.apk' (OS Error: No such file or directory, errno = 2)

[Taknok]

Draft waiting UTF8 revert

[kitadai31]

No, I mean removing it.outputStream().use { } and passing the File to StreamResult() directly
My bad

[Taknok]

val transformer = TransformerFactory.newInstance().newTransformer()
if (isAndroid) {
    transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-16")
}
transformer.transform(DOMSource(this), StreamResult(it))

instead of

it.outputStream().use {
    val transformer = TransformerFactory.newInstance().newTransformer()
    if (isAndroid) {
        transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-16")
    }
    transformer.transform(DOMSource(this), StreamResult(it))
}

?

[kitadai31]

Yes

[Taknok]

Thus, as we need to enforce the UTF-8 conversion, I cannot remove this section as replacing this with StreamResult(file) will remove the control on the encoding, which is precisely what we want to achieve.

Final code would be :

            val writer = StringWriter()
            TransformerFactory.newInstance().newTransformer().apply {
                if (isAndroid) {
                    setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes")
                    setOutputProperty(OutputKeys.ENCODING, "UTF-16")
                }

                transform(DOMSource(this@Document), StreamResult(writer))
            }

            it.outputStream().use { stream ->
                stream.write(writer.toString().toByteArray(Charsets.UTF_8))
            }

Are you ok with it ?

I successfully patch youtube and google phone (+aapt2 v35.0.2) with it:

Youtube:

- Device Info
ReVanced Manager: 1.24.0
Model: Pixel 7
Android version: 15
Supported architectures: arm64-v8a
Root permissions: Yes

- Patch Info
App: com.google.android.youtube v20.12.46 (Suggested: 20.12.46)
Patches version: v5.22.0
Patches added: Default
Patches removed: None
Default patch options changed: None

- Settings
Allow changing patch selection: false
Version compatibility check: true
Show universal patches: false
Patches source: revanced/revanced-patches

- Logs
Reading APK
Decoding app manifest
Loading patches
Deleting existing temporary files directory
Decoding resources
Initializing lookup maps
Executing patches
Applied 53 patches
Compiling modified resources
Aligning APK
Signing APK
Patched APK

Google Phone:

- Device Info
ReVanced Manager: 1.24.0
Model: Pixel 7
Android version: 15
Supported architectures: arm64-v8a
Root permissions: Yes

- Patch Info
App: com.google.android.dialer v172.0.749600042 (Suggested: Any)
Patches version: v1.0.0
Patches added: Default
Patches removed: None
Default patch options changed: None

- Settings
Allow changing patch selection: false
Version compatibility check: true
Show universal patches: false
Patches source: Taknok/revanced-patches

- Logs
Reading APK
Decoding app manifest
Loading patches
Deleting existing temporary files directory
Decoding resources
Initializing lookup maps
Executing patches
Adding '.' to search hint to visually detect patched app
Applied 1 patches
Compiling modified resources
Aligning APK
Signing APK
Patched APK

[Taknok]

We have to use the outputStream() to be able to call write(). As we use outputStream() we have to use "use" to ensure the descriptor is closed

[kitadai31]

In the end, we went back to the approach mentioned in LisoUseInAIKyrios's first comment.

But I'm thinking of using a FileWriter directly instead of using a StringWriter.
Since FileWriter uses the system default encoding, and Android's default encoding is always UTF-8, I expect it to write in UTF-8.

If this were possible, the serialized result would be written directly to a file, so there would be no performance impact.
I'll try this tomorrow

[Taknok]

This function patches youtube and google phone without error:

    override fun close() {
        file?.let {
            if (readerCount[it]!! > 1) {
                throw IllegalStateException(
                    "Two or more instances are currently reading $it." +
                        "To be able to close this instance, no other instances may be reading $it at the same time.",
                )
            } else {
                readerCount.remove(it)
            }

            val transformer = TransformerFactory.newInstance().newTransformer().apply {
                if (isAndroid) {
                    setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes")
                    setOutputProperty(OutputKeys.ENCODING, "UTF-16")
                }
            }

            it.writer().use { writer ->
                transformer.transform(DOMSource(this), StreamResult(writer))
            }
        }
    }

[oSumAtrIX]

Why is

                    setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes")

needed?

[Taknok]

Because we want to have an UTF-8 XML at the end, thus we have to explicitly say to not write the encoding as we perform the conversion : it.writer() set by default a charsets.UTF-8

[oSumAtrIX]

So as I understand the transformer writes as utf16 and then the writer as utf8? Why are we omitting the xml declaration. It doesn't have to do anything with encoding from what I am seeing

[Taknok]

The origin:

1. App
2. Apktool decode XML to UTF-8
3. Java read UTF-8 and set it well in memory, string IB memory handles wells UTF-8. When writing to a file on the string it calls .toCharsArray() which encode it into a 16bits chars list (but some unicode needs overflow and are split into 2 16bits chars)
   4.1 openjdk, when writing, is aware of this potential issue. So it handle the writing by checking if the 16chars is a high point unicode and if the encoding is UTF-8, if yes, it goes to a special flow writing the 16charsunicode.
   4.2 Android is not aware of it, it thinks that it is handling UTF-8 that are only 16chars, if this unicode is unknown, so it writes it as an entity as would do JavaScript.
4. Apktool recompiles the res, it do not understand surrogate unicode, if it reads one (like in Android senario) it crashes.

The fix:

1. In memory we have a UTF-8 string with correct high point emoji.
2. The toStream class will be called and split the strings with toCharsArray() (else we need to rewrite the whole ToStream from xalan, I tried this but there is private class etc in xalan increasing complexity and code copy).
3. We tell to the writer that it will handle 16 bits char (UTF-16), but the charset used will be UTF-8 (default value of the file writers), so the output file should not have an xml header forcing to UTF-18 as we used UTF-8 charset.
4. Aapt2 reads the file with UTF-8 and high point encoded value.

If you want to follow the flow in the debugging mode, you have to set a breaking in the characters() function for the ToStream.java class (called by the xml TreeWalker.java) and follow step by step the code for each case

[kitadai31]

For JRE, there is no need to use Writer
I suggest this code
Also I tried adding some comments, I hope it helps

            val transformer = TransformerFactory.newInstance().newTransformer()
            if (isAndroid) {
                // Set to UTF-16 to prevent surrogate pairs from being escaped to broken numeric character references.
                transformer.setOutputProperty(OutputKeys.ENCODING, "UTF-16")
                // The XML declaration will have encoding="UTF-16", but we're going to write it back in UTF-8, so omit it.
                transformer.setOutputProperty(OutputKeys.OMIT_XML_DECLARATION, "yes")
                // Use FileWriter to output the XML file in UTF-8 encoding.
                it.writer().use { writer ->
                    transformer.transform(DOMSource(this), StreamResult(writer))
                }
            } else {
                transformer.transform(DOMSource(this), StreamResult(it))
            }

[Taknok]

Perf test with youtube and default patch:
Manager main branch + patcher main with surrogate fix : 334s
Manager main branch + patcher main branch vanilla : 349s

[Taknok]

The (slightly) better performance is due to the buffered writer that buffer the chars wrote one by one. A main branch build with a buffered writer (https://github.com/Taknok/revanced-patcher/tree/main-buffered) have a patch time (for youtube with default selection) of 290s

[Taknok]

Hi @kitadai31
This is a gentle reminder about this PR I opened a month ago, no rush at all. Whenever you have a chance to take a look, I'd really appreciate your feedback.

[LisoUseInAIKyrios]

The (slightly) better performance is due to the buffered writer that buffer the chars wrote one by one. A main branch build with a buffered writer (https://github.com/Taknok/revanced-patcher/tree/main-buffered) have a patch time (for youtube with default selection) of 290s

Make a pull request with that one liner change. A ~1 minute reduction in patch time is desirable.

[Taknok]

Euh... ok but the way of how is formulated your message seems that the fix of the surrogate crash is not desirable ? O.o

[LisoUseInAIKyrios]

This PR is definitely desired. I was speaking half off topic about that other change.

I'm not reviewing this PR so someone else needs to approve it.

[kitadai31]

I think this PR is ready

[oSumAtrIX]

If the comment is right, this is fine to merge

[Taknok]

I will rebase this PR on the new dev

[Taknok]

🎉

[kitadai31]

@Taknok Hey, your last commit broke everything!
A use of Writer was removed accidentally.
When UTF-16 is set to the output encoding, it must be output with Writer to make the final output file UTF-8.
So a Writer must be used in isAndroid block.

---

Also, after some testing, I found that OutputStream is same with or without .buffered().
I created a custom FileOutputStream and FileWriter that record the number of writes, and measured the number of times each write was called, with and without wrapping it in Buffered, when output an XML file.

The number of times write() was called while outputting YT's public.xml.
MyFileOutputStream without buffering: 119
MyFileOutputStream with buffering: 119
MyFileWriter without buffering 1461632
MyFileWriter with buffering: 238

This result shows that when constructing a StreamResult, a Writer must be wrapped in Buffered, but an OutputStream does not need to be wrapped.
The XML Transformer is likely doing its own buffering if the target is an OutputStream.
Therefore, #347 (perf: Use a buffered writer to reduce IO overhead) was actually not necessary.

The source code I used to test:
https://gist.github.com/kitadai31/f087562d19a740e43ff472a8afcf623b

This PR basically needs to be reverted to the point of dfb7f05 (Before force-pushed)
I will open a PR to fix these.

[Taknok]

Ok tag me in the PR you open

[kitadai31]

@Taknok PR opened #356

[Taknok]

Thank you