Random crashes when adding / removing / editing in script

[DLPB2]

I am using latest release. I have ruled out everything on my side. I have disabled rendering - still happens.

Previously, I also used a debug build and it still crashes. The crash occurs without any error message or logging, so it's likely failing outside of the main exe code (in a dll/ resource that does not trigger normal error handling).

The crash results in an immediate close down of Makoureactor. I am unsure how to proceed in getting anything more meaningful as a bug report, as there is absolutely nothing to go on.

The crash can happen when adding, removing, editing, or even repositioning operations in script.

[DLPB2]

I'll record a video and post here when it happens again.

[DLPB2]

It has kept happening but recording it has proven to be annoying. Any case, I ran this issue through chatgpt as well. See if any of this is relevant.

Yes. The most probable cause is invalid script opcode indexes being passed into Qt QList operations.

That fits your symptom: instant close, no Makou error, debug build also exits. It may be hitting a Qt assert/abort or invalid container access rather than throwing a normal handled exception. Your GitHub issue says it happens when adding/removing/editing/repositioning script commands, even with rendering disabled.

The suspicious pattern is this:

int OpcodeList::selectedID()
{
    if (currentItem() == nullptr) {
        return -1;
    }

    return currentItem()->data(0, Qt::UserRole).toInt();
}

selectedID() just returns whatever integer is stored on the tree item. selectedIDs() does the same for all selected items, with no validation against _script->size().

Then those IDs are fed directly into script mutation/access functions:

_opcodes.replace(opcodeID, opcode);
_opcodes.removeAt(opcodeID);
_opcodes.insert(opcodeID, opcode);

and:

return _opcodes[opcodeID];
return _opcodes.at(opcodeID);

These need valid indexes. Qt documents that QList::at() / operator[] require 0 <= i < size(), and move/swap indexes must also be within bounds.

The biggest smoking gun is moveOpcode():

bool Script::moveOpcode(qsizetype opcodeID, MoveDirection direction)
{
    if (opcodeID >= _opcodes.size()) {
        return false;
    }

    ...
    _opcodes.swapItemsAt(opcodeID, opcodeID + 1);
    ...
    _opcodes.swapItemsAt(opcodeID, opcodeID - 1);
}

It checks opcodeID >= size, but does not check opcodeID < 0. So -2, -1, or another bad value can reach swapItemsAt(). That is exactly the kind of thing that can make a Qt debug build abort instantly.

And there is an actual -2 value in the UI. When the script is empty, fill() creates an informational placeholder item and stores -2 in Qt::UserRole:

item->setFlags(Qt::NoItemFlags);
item->setData(0, Qt::UserRole, -2);

Then actions are enabled afterwards. So if the current item / selected item state ever points at that placeholder, or stale tree data after a rebuild, commands like move, paste, edit, copy, or delete can receive a bad ID.

Paste has the same kind of bug:

int opcodeID = selectedID() + 1;
...
_script->insertOpcode(i, opcode);

If selectedID() is -2, paste inserts at -1. If it is stale/out of range, it inserts at a bad position.

Delete/copy/edit also trust indexes:

oldVersions.prepend(_script->opcode(quint16(selectedIDs.at(i))));
_script->removeOpcode(quint16(selectedIDs.at(i)));

and:

opcodeCopied.append(_script->opcode(id));

and edit does:

oldVersion = _script->opcode(opcodeID);

without checking that opcodeID is between 0 and _script->size() - 1.

So the likely fix is to harden both layers.

In Script::moveOpcode() at minimum:

bool Script::moveOpcode(qsizetype opcodeID, MoveDirection direction)
{
    if (opcodeID < 0 || opcodeID >= _opcodes.size()) {
        return false;
    }

    if (direction == Down) {
        if (opcodeID >= _opcodes.size() - 1) {
            return false;
        }

        _opcodes.swapItemsAt(opcodeID, opcodeID + 1);
    } else {
        if (opcodeID <= 0) {
            return false;
        }

        _opcodes.swapItemsAt(opcodeID, opcodeID - 1);
    }

    return true;
}

And every caller should validate IDs before using them:

bool OpcodeList::isValidOpcodeID(int id) const
{
    return _script != nullptr && id >= 0 && id < _script->size();
}

Then:

int opcodeID = selectedID();

if (!isValidOpcodeID(opcodeID)) {
    return;
}

For paste:

int selected = selectedID();
int opcodeID = isValidOpcodeID(selected) ? selected + 1 : _script->size();

if (opcodeID < 0 || opcodeID > _script->size()) {
    opcodeID = _script->size();
}

There is a second suspect too: ScriptEditor creates a hidden/fake QOpenGLWidget when OpenGL is enabled, with a comment saying it is there “to prevent crashes”. That is worth testing separately, but because your crash also happens on copy/delete/move-style operations, the bad opcode index path is the first thing I would fix.

Bluntly: the script editor code trusts tree item IDs too much. After copy/paste/delete/add/fill/rebuild operations, one stale, placeholder, or out-of-range ID can reach QList, and that would explain the random instant shutdown perfectly.