Skip to content

Commit

Permalink
controllers: Fix crash if controller mapping has no module
Browse files Browse the repository at this point in the history
This fixes two bugs that were caused by mixxxdj#2868:

    1. Mixxx didn't check if the JS module path was really a file. If
       you pass in a directory, QtJSEngine::importModule makes Mixxx
       crash (at least on Qt 5.15.0)
    2. If a mapping does not have JS module associated with it, then the
       moduleFileInfo always pointed to the controller directory and
       hence triggered bug 1.

This fixes both of these issues and adds some debug assertions to make
sure it stays like that.
  • Loading branch information
Holzhaus committed Jul 16, 2020
1 parent 1bcd0ab commit b68547c
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 3 deletions.
6 changes: 5 additions & 1 deletion src/controllers/controller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ bool Controller::applyPreset(bool initializeScripts) {
m_pEngine->initializeScripts(scriptFiles);
}

if (initializeScripts) {
// QFileInfo does not have a isValid/isEmpty/isNull method to check if it
// actually contains a reference, so we check if the filePath is empty as a
// workaround.
// See https://stackoverflow.com/a/45652741/1455128 for details.
if (initializeScripts && !pPreset->moduleFileInfo().filePath().isEmpty()) {
m_pEngine->loadModule(pPreset->moduleFileInfo());
}
return success;
Expand Down
13 changes: 12 additions & 1 deletion src/controllers/controllerpresetfilehandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,18 @@ void ControllerPresetFileHandler::addScriptFilesToPreset(
}

QString moduleFileName = controller.firstChildElement("module").text();
preset->setModuleFileInfo(preset->dirPath().absoluteFilePath(moduleFileName));

if (moduleFileName.isEmpty()) {
return;
}

QFileInfo moduleFileInfo(preset->dirPath().absoluteFilePath(moduleFileName));
if (!moduleFileInfo.isFile()) {
qWarning() << "Controller Module is not a file:" << moduleFileInfo.absoluteFilePath();
return;
}

preset->setModuleFileInfo(moduleFileInfo);
}

bool ControllerPresetFileHandler::writeDocument(
Expand Down
20 changes: 19 additions & 1 deletion src/controllers/engine/controllerengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,17 @@ void ControllerEngine::uninitializeScriptEngine() {
}

void ControllerEngine::loadModule(QFileInfo moduleFileInfo) {
// QFileInfo does not have a isValid/isEmpty/isNull method to check if it
// actually contains a reference, so we check if the filePath is empty as a
// workaround.
// See https://stackoverflow.com/a/45652741/1455128 for details.
VERIFY_OR_DEBUG_ASSERT(!moduleFileInfo.filePath().isEmpty()) {
return;
}

VERIFY_OR_DEBUG_ASSERT(moduleFileInfo.isFile()) {
return;
}
#if QT_VERSION >= QT_VERSION_CHECK(5, 12, 0)
m_moduleFileInfo = moduleFileInfo;

Expand Down Expand Up @@ -318,7 +329,14 @@ void ControllerEngine::reloadScripts() {

qDebug() << "Re-initializing scripts";
initializeScripts(m_lastScriptFiles);
loadModule(m_moduleFileInfo);

// QFileInfo does not have a isValid/isEmpty/isNull method to check if it
// actually contains a reference, so we check if the filePath is empty as a
// workaround.
// See https://stackoverflow.com/a/45652741/1455128 for details.
if (!m_moduleFileInfo.filePath().isEmpty()) {
loadModule(m_moduleFileInfo);
}
}

void ControllerEngine::initializeScripts(const QList<ControllerPreset::ScriptFileInfo>& scripts) {
Expand Down

0 comments on commit b68547c

Please sign in to comment.