search

HearthSim / Hearthstone-Deck-Tracker

A deck tracker and deck manager for Hearthstone on Windows
https://hsreplay.net/downloads/
4.61k stars 1.11k forks source link

Enforced null checking removes plugin handler #4360

Closed sgkoishi closed 2 years ago

sgkoishi commented 2 years ago

https://github.com/HearthSim/Hearthstone-Deck-Tracker/blob/4ff343930e19709d2b0eb4a0f0f71dc13a27870a/Hearthstone%20Deck%20Tracker/API/ActionList.cs#L23-L28

The plugin here is null if the handler was not registered inside the class that inherits IPlugin. For example, the PackTracker register it inside a watcher.

https://github.com/HearthSim/Hearthstone-Deck-Tracker/blob/4ff343930e19709d2b0eb4a0f0f71dc13a27870a/Hearthstone%20Deck%20Tracker/API/ActionList.cs#L37-L41 This handler will be removed later by the null checking since the plugin is null.

Not sure if this (register events inside the Plugin class) is intended or not but it was not mentioned elsewhere

If this is intended, also note that the handler can not be registered inside the constructor of the Plugin class. The constructor is called when the plugin itself does not exist in the plugin list yet.

azeier commented 2 years ago

I reverted the change.

While the old behaviour is not ideal, where plugins are able to register actions without being identifiable as such, the change to allow action registration in the plugin constructor is not trivial.

sgkoishi commented 2 years ago

A potential solution (for finding the owner) may be comparing the Assembly instead of type to find the corresponding plugin. It does not work well with multiple plugins inside one Assembly, but that's not common (and maybe not expected?)

azeier commented 2 years ago

I believe the problem isn't the type, iterating through the stack frames should work fine in most cases (though it's not doing that currently). It's what you mentioned at the bottom of the post: plugins need to be instantiated to be added to PluginManager.Instance.Plugins. However instantiating means their ctor is called and they can call ActionList.Add before being added. I'm sure there's ways around that as well but it doesn't seem worth the hassle for now. This change wasn't meant to improve plugin security, it just got caught in the nullable refactor.

I think the original intention was for plugins to register for events in OnLoaded, but that's not documented or enforced anywhere.