MantisBT - Doomseeker
View Issue Details
0003401Doomseeker[All Projects] Bugpublic2018-03-05 22:592019-07-30 10:13
Zalewa 
Pol M 
lowmajoralways
closedfixed 
MicrosoftWindowsXP/Vista/7
 
1.31.3 
0003401: DOOMSEEKER_ABI mismatching plugins still pop up unknown procedure errors.
DOOMSEEKER_ABI is meant to gracefully handle plugins with mismatching ABI. We bump the ABI, and plugins with mismatching ABI shouldn't load. Instead, plugins with mismatching ABI will still pop up errors on Windows. These errors are probably generated by LoadLibrary(), though more info should be gathered on that.
1. Windows 7.
2. Change ABI, for example change Player(params) constructor.
3. Bump DOOMSEEKER_ABI definition.
4. Build only "doomseeker" target.
5. Try to run "doomseeker.exe".
6. Get spammed with popups telling you that Player(params) procedure is unknown - one popup for each plugin.
No tags attached.
parent of 0003644closed Pol M No ABI versioning is displayed in the about dialog 
Issue History
2018-03-05 22:59ZalewaNew Issue
2018-10-05 07:22WubTheCaptainPrioritynormal => low
2019-03-31 15:43Pol MAssigned To => Pol M
2019-03-31 15:43Pol MStatusnew => assigned
2019-03-31 15:46Pol MNote Added: 0020461
2019-03-31 15:59Pol MNote Edited: 0020461bug_revision_view_page.php?bugnote_id=20461#r12463
2019-03-31 15:59Pol MStatusassigned => needs testing
2019-04-03 11:07ZalewaNote Added: 0020470
2019-04-03 13:14Pol MNote Added: 0020471
2019-04-03 13:14Pol MStatusneeds testing => assigned
2019-04-16 18:38Pol MNote Added: 0020496
2019-04-16 18:39Pol MStatusassigned => needs testing
2019-04-20 06:55ZalewaNote Added: 0020514
2019-04-20 06:55ZalewaNote Edited: 0020514bug_revision_view_page.php?bugnote_id=20514#r12482
2019-04-20 06:56ZalewaNote Edited: 0020514bug_revision_view_page.php?bugnote_id=20514#r12483
2019-04-20 07:26ZalewaNote Added: 0020515
2019-04-20 07:27ZalewaStatusneeds testing => feedback
2019-04-20 11:53Pol MNote Added: 0020517
2019-04-21 13:27ZalewaNote Added: 0020520
2019-04-21 13:27ZalewaStatusfeedback => assigned
2019-04-21 16:05Pol MNote Added: 0020524
2019-04-22 06:24ZalewaNote Added: 0020528
2019-04-27 15:28Pol MNote Added: 0020555
2019-04-27 15:28Pol MStatusassigned => needs review
2019-05-01 09:43ZalewaNote Added: 0020587
2019-05-01 09:43ZalewaStatusneeds review => assigned
2019-05-01 17:01WubTheCaptainTarget Version => 1.3
2019-05-01 17:04WubTheCaptainRelationship addedparent of 0003644
2019-05-01 19:49Pol MNote Added: 0020602
2019-05-02 09:11WubTheCaptainStatusassigned => needs review
2019-05-02 12:27ZalewaNote Added: 0020604
2019-05-02 12:29ZalewaNote Edited: 0020604bug_revision_view_page.php?bugnote_id=20604#r12549
2019-05-20 15:37Pol MStatusneeds review => needs testing
2019-06-23 09:43ZalewaStatusneeds testing => resolved
2019-06-23 09:43ZalewaFixed in Version => 1.3
2019-06-23 09:43ZalewaResolutionopen => fixed
2019-07-30 10:13WubTheCaptainStatusresolved => closed

Notes
(0020461)
Pol M   
2019-03-31 15:46   
(edited on: 2019-03-31 15:59)
Looks like right now it only shows two similar error messages. I'm gonna disable one of them, and leave the other. Following the "steps to reproduce", I have not managed to get said results.
pr

(0020470)
Zalewa   
2019-04-03 11:07   
The pr is not enough as the problem is directly related to Windows loading the DLL and auto-generating the pop-ups. The displayed errors are not generated by us. I don't even know if this is solvable without parsing the DLL's import table before passing them to LoadLibrary() but maybe WinAPI provides some method of capturing those rough Windows errors and handling them gracefully.
(0020471)
Pol M   
2019-04-03 13:14   
Oh! That explains why on wine it was working. I'll investigate this issue on a native windows installation.
(0020496)
Pol M   
2019-04-16 18:38   
Okay, I've found an easy way to handle this: SetErrorMode
Unfortunately it won't allow us to know what errors are we suppressing. That said, since we can disable exclusively errors related to the engines, I'm not that worried about suppressing important info.
PR
(0020514)
Zalewa   
2019-04-20 06:55   
(edited on: 2019-04-20 06:56)
Oh the woes of see plus plus. The commit that disables SEM_FAILCRITICALERRORS does get rid of the invasive popup dialog boxes indeed. However, the
#define DOOMSEEKER_ABI_VERSION
still remains useless on Windows.

Let's see: a DLL with an incompatible ABI refuses to load, right? If it refuses to load, then we cannot compare the ABI in the DLL to the #define in Doomseeker. Therefore, the #define has no chance to do anything. However, if you think about it, does it really need to? Let's consider:

1. Since Windows already detects ABI incompatibility for us we already get the "feature" of an incompatible DLL not being loaded.

2. Since DLLs are not loaded we're already safe from incompatibilities causing crashes.

3. The obnoxious error popups are we gone. There's now a somewhat cryptic "error 127" in the logs, which is not so bad for programmers, but most users won't know what it means. We could improve that a bit by checking if GetLastError() returns ERROR_PROC_NOT_FOUND and print a better log message that directly states that the plugin is not in the correct version.

4. With the unusable #define what we can't do is to display Doomseeker's and plugin's ABI version numbers so that the user knows which one is newer. But, how useful would that be, really?

(0020515)
Zalewa   
2019-04-20 07:26   
I merged the PR as the commit here as it is a step in the right direction.
(0020517)
Pol M   
2019-04-20 11:53   
Quote from Zalewa

We could improve that a bit by checking if GetLastError() returns ERROR_PROC_NOT_FOUND and print a better log message that directly states that the plugin is not in the correct version.

That's an excellent idea. I'd manually add an exception to code 127 and add something like "perhaps this is an old engine? Check for newer updates!" (Something that kinda states what the problem could be and how to solve it)

Quote from Zalewa

 what we can't do is to display Doomseeker's and plugin's ABI version numbers so that the user knows which one is newer. But, how useful would that be, really?

While it's true that it would be interesting to show the info, the end user will solve the problem by updating anyway, so I'd say that it is not that bad that they lose that info. I find it highly unlikely that someone would downgrade to an older version of doomseeker and use newer engines, so the issue will almost always appear when people update doomseeker partially.
(0020520)
Zalewa   
2019-04-21 13:27   
The actual problems with ABI incompatibility will arise if someone decides to release a plugin that will be hosted outside of Doomseeker's source control and Doomseeker's release cycle. In such case it may be useful for the user to know if plugin is for an older or a newer version of Doomseeker.

If such plugin appears we will also need to be much more rigorous about Doomseeker's ABI instead of breaking it freely whenever the current Plugin API gets in the way of the development.
(0020524)
Pol M   
2019-04-21 16:05   
Quote from Zalewa

In such case it may be useful for the user to know if plugin is for an older or a newer version of Doomseeker.

Yeah, that will be a small problem. That said, I'd imagine that the developer will list the compatible doomseeker versions, and the slow release cycle will help us here, giving said developer time to adjust to the new ABI or mark the new Doomseeker version as compatible. It's a shame that the info will not be displayed, because it's very likely that they will use Windows to develop plugins. To mitigate a little the problem for developers (since the way I see it are the most afected) and maybe even players, we could say what the ABI is in the "About doomseeker" section, maybe at the left of the version? Like "1.3 (ABI: 3)". That would allow said developer to know very easily if the plugin needs a new version.
Quote from Zalewa

 will also need to be much more rigorous about Doomseeker's ABI

No problem. If the API gets on the way, we'll be more careful. Ticket 0001961 will do so (I have not been careful at all with ABI compatibility, so my guess is that it will).
(0020528)
Zalewa   
2019-04-22 06:24   
Displaying the ABI in the about dialog is not a bad idea.
(0020555)
Pol M   
2019-04-27 15:28   
PR
(0020587)
Zalewa   
2019-05-01 09:43   
PR merged
The last thing that remains to be done here is to display the "perhaps plugin is for a different Doomseeker version" message in the log.
(0020602)
Pol M   
2019-05-01 19:49   
PR :)
(0020604)
Zalewa   
2019-05-02 12:27   
(edited on: 2019-05-02 12:29)
Cleaned up the PR and merged it here.

Also, added a follow-up commit that handles the `%1` formatting placeholders that are present in WinAPI messages. It's probable that the word "Plugin" will fit under the %1 so I naively just substitute this. You can produce a message that normally has this placeholder by creating an empty 'libsomething.dll' file in the "engines/" directory.

In any case, the gist of the ticket is fulfilled and I don't see any other problem with current implementation so I consider it as resolved.

I'll push the changelog update in a bulk commit after my own work is done.