MantisBT - Doomseeker |
View Issue Details |
|
ID | Project | Category | View Status | Date Submitted | Last Update |
0003401 | Doomseeker | [All Projects] Bug | public | 2018-03-05 22:59 | 2019-07-30 10:13 |
|
Reporter | Zalewa | |
Assigned To | Pol M | |
Priority | low | Severity | major | Reproducibility | always |
Status | closed | Resolution | fixed | |
Platform | Microsoft | OS | Windows | OS Version | XP/Vista/7 |
Product Version | | |
Target Version | 1.3 | Fixed in Version | 1.3 | |
|
Summary | 0003401: DOOMSEEKER_ABI mismatching plugins still pop up unknown procedure errors. |
Description | 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. |
Steps To Reproduce | 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. |
Additional Information | |
Tags | No tags attached. |
Relationships | parent of | 0003644 | closed | Pol M | No ABI versioning is displayed in the about dialog |
|
Attached Files | |
|
Issue History |
Date Modified | Username | Field | Change |
2018-03-05 22:59 | Zalewa | New Issue | |
2018-10-05 07:22 | WubTheCaptain | Priority | normal => low |
2019-03-31 15:43 | Pol M | Assigned To | => Pol M |
2019-03-31 15:43 | Pol M | Status | new => assigned |
2019-03-31 15:46 | Pol M | Note Added: 0020461 | |
2019-03-31 15:59 | Pol M | Note Edited: 0020461 | bug_revision_view_page.php?bugnote_id=20461#r12463 |
2019-03-31 15:59 | Pol M | Status | assigned => needs testing |
2019-04-03 11:07 | Zalewa | Note Added: 0020470 | |
2019-04-03 13:14 | Pol M | Note Added: 0020471 | |
2019-04-03 13:14 | Pol M | Status | needs testing => assigned |
2019-04-16 18:38 | Pol M | Note Added: 0020496 | |
2019-04-16 18:39 | Pol M | Status | assigned => needs testing |
2019-04-20 06:55 | Zalewa | Note Added: 0020514 | |
2019-04-20 06:55 | Zalewa | Note Edited: 0020514 | bug_revision_view_page.php?bugnote_id=20514#r12482 |
2019-04-20 06:56 | Zalewa | Note Edited: 0020514 | bug_revision_view_page.php?bugnote_id=20514#r12483 |
2019-04-20 07:26 | Zalewa | Note Added: 0020515 | |
2019-04-20 07:27 | Zalewa | Status | needs testing => feedback |
2019-04-20 11:53 | Pol M | Note Added: 0020517 | |
2019-04-21 13:27 | Zalewa | Note Added: 0020520 | |
2019-04-21 13:27 | Zalewa | Status | feedback => assigned |
2019-04-21 16:05 | Pol M | Note Added: 0020524 | |
2019-04-22 06:24 | Zalewa | Note Added: 0020528 | |
2019-04-27 15:28 | Pol M | Note Added: 0020555 | |
2019-04-27 15:28 | Pol M | Status | assigned => needs review |
2019-05-01 09:43 | Zalewa | Note Added: 0020587 | |
2019-05-01 09:43 | Zalewa | Status | needs review => assigned |
2019-05-01 17:01 | WubTheCaptain | Target Version | => 1.3 |
2019-05-01 17:04 | WubTheCaptain | Relationship added | parent of 0003644 |
2019-05-01 19:49 | Pol M | Note Added: 0020602 | |
2019-05-02 09:11 | WubTheCaptain | Status | assigned => needs review |
2019-05-02 12:27 | Zalewa | Note Added: 0020604 | |
2019-05-02 12:29 | Zalewa | Note Edited: 0020604 | bug_revision_view_page.php?bugnote_id=20604#r12549 |
2019-05-20 15:37 | Pol M | Status | needs review => needs testing |
2019-06-23 09:43 | Zalewa | Status | needs testing => resolved |
2019-06-23 09:43 | Zalewa | Fixed in Version | => 1.3 |
2019-06-23 09:43 | Zalewa | Resolution | open => fixed |
2019-07-30 10:13 | WubTheCaptain | Status | resolved => 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
|
|
|
|
(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
|
|
|
|
(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
|
|
|
|
(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.
|
|