SCI game "Manic Mansion" doesn't work on current build

[lordelan]

As described ​here the game Manic Mansion (not to be confused with Maniac Mansion) doesn't work.

It crashes with the console error:

WARNING: SCI [SCI0, SCI01, SCI10, SCI11, SCI32] failed to instantiate engine: Game data not found (target 'manic-demo', path 'C:\Tools\ScummVM\manic\')! 

As user Raziel assumed, this might be due to the SCI engine overhaul that happened some time ago.

[digitall]

Just tried this with the latest git master and this crashes on Linux x86_64 with a segmentation fault.

Running with valgrind gives the following backtrace:

==1826== Invalid read of size 8
==1826==    at 0x3F9B61: AdvancedMetaEngine::toDetectedGame(ADDetectedGame const&) const (advancedDetector.cpp:94)
==1826==    by 0x3FA239: AdvancedMetaEngine::detectGames(Common::FSList const&) const (advancedDetector.cpp:178)
==1826==    by 0x1C01DB: EngineManager::detectGames(Common::FSList const&) const (plugins.cpp:527)
==1826==    by 0x32B030: GUI::LauncherDialog::doGameDetection(Common::String const&) (launcher.cpp:554)
==1826==    by 0x32A165: GUI::LauncherDialog::addGame() (launcher.cpp:389)
==1826==    by 0x32B5C4: GUI::LauncherDialog::handleCommand(GUI::CommandSender*, unsigned int, unsigned int) (launcher.cpp:618)
==1826==    by 0x36D48E: GUI::CommandSender::sendCommand(unsigned int, unsigned int) (object.h:55)
==1826==    by 0x36ADC5: GUI::ButtonWidget::handleMouseUp(int, int, int, int) (widget.cpp:332)
==1826==    by 0x31F861: GUI::Dialog::handleMouseUp(int, int, int, int) (dialog.cpp:226)
==1826==    by 0x3270C3: GUI::GuiManager::processEvent(Common::Event const&, GUI::Dialog*) (gui-manager.cpp:588)
==1826==    by 0x326487: GUI::GuiManager::runLoop() (gui-manager.cpp:359)
==1826==    by 0x31F1DF: GUI::Dialog::runModal() (dialog.cpp:80)
==1826==  Address 0x8 is not stack'd, malloc'd or (recently) free'd

Will take a look at this and see if I can work out why this occurs.

[digitall]

Well, firstly there is no FANMADE detection entry in the SCI engine for Manic Mansion:
​https://github.com/scummvm/scummvm/blob/master/engines/sci/detection_tables.h#L5091

One should be added, but this should still not crash in this way on unknown SCI fangames...
The backtrace from valgrind indicates that this is occurring when the fallback detector is being invoked. It may not be setting some of the plainGameDescriptor values correctly. Will need to track this down and fix before adding a detection entry for this game i.e. so we are fixing both issues and not just masking the crash.

[digitall]

Have tried looking at this by checking when this regression was introduced.
v2.0.0 does not exhibit the bug, but master does.
Will do a git bisection...

[digitall]

Right... Bisection shows that regression occurred between 9587dd5c21d388616dc8d42db909390fab384c2f (good) and 90b78c544657bf0fc41d6b86276a0873060345b5 (bad). Unfortunately the intermediate commits will not build due to:
engines/advancedDetector.cpp: In member function ‘virtual DetectedGames AdvancedMetaEngine::detectGames(const Common::FSList&) const’:
engines/advancedDetector.cpp:199:25: error: ‘struct DetectedGame’ has no member named ‘preferredTarget’

fallbackDetectedGame.preferredTarget += "-fallback";

Will look at patching the unbuildable commits and narrowing this down, but I think I know what is going on and where a patch should be applied.

[digitall]

I suspect the fix will be:

diff --git a/engines/advancedDetector.cpp b/engines/advancedDetector.cpp
index 6f4efcfb57..26b06b6417 100644
--- a/engines/advancedDetector.cpp
+++ b/engines/advancedDetector.cpp
@@ -91,7 +91,11 @@ DetectedGame AdvancedMetaEngine::toDetectedGame(const ADDetec
tedGame &adGame) co
                extra = "";
        } else {
                const PlainGameDescriptor *pgd = findPlainGameDescriptor(desc->g
ameId, _gameIds);
-               title = pgd->description;
+               if (pgd) {
+                       title = pgd->description;
+               } else {
+                       title = "";
+               }
                extra = desc->extra;
        }

The findPlainGameDescriptor returns nullptr when the game id is not found in the plain game id list supplied by the meta engine, thus dereferencing this causes a segfault crash.

I thought that the issue might be that desc->gameId should be gameId in the findPlainGameDescriptor call, but testing shows that removes the game name in some cases, so better to just fix the code to supply an empty title in these cases which is what the original "good" cases did.

[digitall]

Right. Patched out the broken line of code as not critical for testing and have located regression to commit 90b78c544657bf0fc41d6b86276a0873060345b5:

commit 90b78c544657bf0fc41d6b86276a0873060345b5
Author: Bastien Bouclet <bastien.bouclet@gmail.com>
Date:   Sun May 6 15:51:03 2018 +0200

    ENGINES: Merge GameDescriptor and DetectedGame

[digitall]

OK. Looked at the previous code for this and I can see how it previously returned an empty string in this case so my patch is correct. Have commited as 5a29aa8812ca523f79a5ca5b492862762dbd19e4.

This should allow Manic Mansion and other SCI fangames to be detected and added by fallback.

Will also look at adding a specific fangame detection entry for Manic Mansion as well to close this out.

[digitall]

Detection entry added in commit c96b4cca4d6b839442f282c651a0eb32396e95bd.

Closing as fixed.