pardeike / Harmony

A library for patching, replacing and decorating .NET and Mono methods during runtime
https://www.patreon.com/pardeike
MIT License
5.17k stars 486 forks source link

InvalidCastException on reverting the patches #102

Closed dymanoid closed 6 years ago

dymanoid commented 6 years ago

I have a mod for the Cities: Skylines game. I use Harmony version 1.1.0 for creating a couple of patches for the game methods.

I activate my patches using attributes:

myHarmonyInstance.PatchAll(Assembly.GetExecutingAssembly());

But before doing that, I want to ensure that the methods are only patched once. The game might call the mod's initialization methods multiple times (unexpectedly), so I created a kind of a clean-up before running my patches:

foreach (MethodBase method in harmonyInstance.GetPatchedMethods().ToList())
{
    harmonyInstance.RemovePatch(method, HarmonyPatchType.All, "my-custom-id");
}

Of course, I want to remove only my patches, hence I use the overload that specifies my custom ID.

But in case there are already some patches that are applied by other developers (and maybe using other Harmony versions), I get an exception:

System.InvalidCastException: Cannot cast from source type to destination type.
  at Harmony.PatchInfoSerialization.Deserialize (System.Byte[] bytes)
  at Harmony.HarmonySharedState.GetPatchInfo (System.Reflection.MethodBase method)
  at Harmony.PatchProcessor.Unpatch (HarmonyPatchType type, System.String harmonyID) 
  at Harmony.HarmonyInstance.RemovePatch 

Note: this also happens when I clean-up the patches on unloading the mod, because I use the same foreach loop with RemovePatch calls.

pardeike commented 6 years ago

Is this only happening if older Harmony versions are used in parallel with your v1.1?

dymanoid commented 6 years ago

Unfortunately I have no info whether the players use other (older) Harmony versions with some other mods. This was my guess. There are hundreds of mods for the game, and many of them use either the "direct" method patching or various Harmony versions. So I thought there might be some "very old" Harmony patch hanging on that method which causes the cast exception.

I tried to install some mods that use various Harmony versions, but could not reproduce the issue on my machine. This might be some edge case or maybe an unlucky combination of "direct" patching, different Harmony versions, and assembly recompile made by the game (Mono).

pardeike commented 6 years ago

I need to close this for now as there is no reasonable way to reproduce this for me. Feel free to open the issue again if you get more information. The deserialisation should work across different versions of Harmony since the mechanics are the same as for patching a method (each patch is combined by deserialising PatchInfo, adding to it, then serialising it again to save it).

dymanoid commented 6 years ago

Just for the history if someone discovers similar errors.

The InvalidCastException in the method mentioned above can be thrown if the Harmony assembly is loaded more than once into the same AppDomain. Then, the runtime considers the types as different types (from different assemblies) even they are actually the same.

Actually, it's a pretty common use case in the Cities: Skylines game, because many modders deliver their mods with a Harmony assembly, and the game loads the mods using Assembly.LoadFile. This allows many copies of an assembly even with same name and same version. A very dangerous and unstable environment!

pardeike commented 6 years ago

Just for the record: in RimWorld, all mods that use Harmony package their own version and there is even a common mod that contains Harmony that other mods depend on. Loading the same dll will not load it twice as the Windows dll cache lookup will prevent loading the same dll twice. Different versions of Harmony will load separately but Harmony was designed to allow this. All this works for many months now in RimWorld without problems.

dymanoid commented 6 years ago

...but not in Cities: Skylines, sadly. There are many, many users complaining about issues caused by installing at least two mods that use Harmony. Some people even report a complete application freeze when two mods with Harmony are used: one of them using version 1.1.0 and the other using yet-unreleased latest HEAD. E.g. this mod (not mine).

By the way, loading the same assembly multiple times in one AppDomain is possible both using Assembly.LoadFile and Assembly.LoadFrom. Please refer to the documentation. This has nothing to do with Windows' cache. It's a pity that you overlooked that. But I doubt there's anything you can do about it, because assembly loading is governed by the Framework. Maybe some black magic with AppDomains or whatever... I've never had to do with such problems like having multiple assembly copies in one AppDomain. According to Microsoft, it's possible but it's also a state to be avoided. Sadly, Cities: Skylines does nothing to prevent it.

pardeike commented 6 years ago

The AppDomain is the cache: https://stackoverflow.com/questions/5454523/does-assembly-load-use-cache

RimWorld uses Assembly.LoadFile and in my tests you get back the same dll and not a copy.

dymanoid commented 6 years ago

The fact that one application loads assemblies in some way doesn't imply that all other applications do the same. Cities: Skylines uses its own logic to load the mods' assemblies, including such methods as Assembly.ReflectionOnlyLoadFrom(string) and Assembly.Load(byte[]). The dependencies are resolved manually and not by using default runtime binder. This leads to a state where same assemblies are loaded multiple times into a single AppDomain (partly because of Colossal Order's spaghetti code).

I don't want to debate with you on this topic. I just wanted to keep this information for other developers.

Acc3ssViolation commented 6 years ago

I've had a user of my mod RandomTrainTrailers experience the same issue, but unfortunately I've not been able to reproduce it yet.

Here's my user's error

The Mod C:\Games\Steam\steamapps\workshop\content\255710\870291141 [0Harmony.dll, RandomTrainTrailers.dll] has caused an error [ModException]

Details:
System.InvalidCastException: Cannot cast from source type to destination type.
at Harmony.PatchInfoSerialization.Deserialize (System.Byte[] bytes) [0x00000] in <filename unknown>:0 
at Harmony.HarmonySharedState.GetPatchInfo (System.Reflection.MethodBase method) [0x00000] in <filename unknown>:0 
at Harmony.HarmonyInstance+<>c__DisplayClass13_0.<VersionInfo>b__0 (System.Reflection.MethodBase method) [0x00000] in <filename unknown>:0 
at Harmony.CollectionExtensions.Do[MethodBase] (IEnumerable`1 sequence, System.Action`1 action)[0x00000] in <filename unknown>:0 at Harmony.HarmonyInstance.VersionInfo (System.Version

Also I noticed the MSDN page for Assembly.Load(byte[]) does indicate it will always return "a new Assembly object with its own mapping" and since C:S uses that for loading it might be related to the issue we're having. I've made a small mod that logs loaded mods and assemblies in the current AppDomain when loading the level, maybe that can shed some light on the duplicate loading that seems to be happening.

So yeah, not that much in the way of possible fixes but it does show that multiple people are having issues. And since Harmony is a pretty good replacement for basic detouring it'd be a shame if we can't find a fix or workaround for this.

pardeike commented 6 years ago

Maybe the game should load dlls different then? Or maybe not let the game load Harmony? Another possibility is to merge dlls - your mod dll and Harmony dll into one. That should work.

Harmony versions can coexist. It does not matter if it's "[Harmony 1.0.9.1] [Harmony 1.1]" or "[Harmony 1.0.9.1] [Harmony 1.0.9.1] [Harmony 1.0.9.1] [Harmony 1.1] [Harmony 1.1]". The whole serialize, deserialize thing is exactly designed for this. Maybe the dll lookup mechanism isn't correctly set up. I had someone earlier add some extra code in his mod to make it find all the potential classes for deserialisation. Can't remember exactly what it was but it worked for him.

pardeike commented 6 years ago

Since this is popular, I reopen the issue.

pardeike commented 6 years ago

I think it was something about the assembly resolver: https://msdn.microsoft.com/en-us/library/system.appdomain.assemblyresolve.aspx

dymanoid commented 6 years ago

Cities: Skylines doesn't use this. It creates a dictionary of assemblies and manually searches for dependencies. It's not a neat solution, but it somehow works. At least, it worked until Harmony's got involved.

Just a random thought. What if a modder built a Harmony assembly by themselves that has an existing version number (say, 1.1.0.0) but is based on another commit than the official tag? There might be some incompatibilities maybe. But I doubt this might be the reason. If a type cannot be cast to itself, it's a clean indication that there are multiple instances of one assembly in the AppDomain. At least according to Microsoft.

We also shouldn't forget that it all runs on Mono. Could Mono have some issues here?

pardeike commented 6 years ago

Any progress on this? I am inclined to close this issue.

Acc3ssViolation commented 6 years ago

Unfortunately I haven't been able to find out anything as I can't seem to replicate the issue myself.

pardeike commented 6 years ago

Let’s close this for now. Please open a new issue if such an issue happens with the latest release or the master. Thank you.

Nytra commented 7 months ago

I'm running into this problem in Harmony v2.2.2. I am doing some funky hot reloading of the assembly into the same AppDomain and unpatching / repatching. I have no idea what is the actual cause of this because I had it working for a bit but then changed some code in some of my patches and now it's not working again.

The exception that I'm getting:

Exception:
System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.InvalidCastException: Specified cast is not valid.
  at HarmonyLib.Patch.get_PatchMethod () [0x00079] in <255414689d344b1385ae719f6cce8c55>:0 
  at HarmonyLib.Harmony+<>c__DisplayClass13_1.<UnpatchAll>b__1 (HarmonyLib.Patch patchInfo) [0x00000] in <255414689d344b1385ae719f6cce8c55>:0 
  at HarmonyLib.CollectionExtensions.Do[T] (System.Collections.Generic.IEnumerable`1[T] sequence, System.Action`1[T] action) [0x00014] in <255414689d344b1385ae719f6cce8c55>:0 
  at HarmonyLib.CollectionExtensions.DoIf[T] (System.Collections.Generic.IEnumerable`1[T] sequence, System.Func`2[T,TResult] condition, System.Action`1[T] action) [0x00007] in <255414689d344b1385ae719f6cce8c55>:0 
  at HarmonyLib.Harmony.UnpatchAll (System.String harmonyID) [0x00059] in <255414689d344b1385ae719f6cce8c55>:0 
pardeike commented 7 months ago

I'm running into this problem in Harmony v2.2.2. I am doing some funky hot reloading of the assembly into the same AppDomain and unpatching / repatching. I have no idea what is the actual cause of this because I had it working for a bit but then changed some code in some of my patches and now it's not working again.

That scenario is not supported by Harmony. It’s pretty hard or even impossible to reload assemblies with or without Harmony. It is also unclear if this issue is related to the current thread.

Nytra commented 7 months ago

I figured out that for my issue specifically there was a anonymous delegate defined inside of the patch that was for some reason preventing the patch from being removed. I got rid of that delegate and the problem resolved. It was a small anonymous delegate with only one or two lines of code inside. I guess it was being 'optimized' by the compiler or the JIT or something.

Prior to removing that it was able to work the first time and after first reload but on subsequent reloads it would break. Now it just works all the time.