Direct access to ControlObject in midiobject may not be thread safe...

Bug #302684 reported by ironstorm on 2008-11-27
Affects Status Importance Assigned to Milestone
Albert Santoni

Bug Description

During the merge of Tom's MIDI branch, the following patch came to my attention, I noticed that this method replaces a thread safe call to ControlObjectThread with a direct call to a ControlObject->setValue (p is a ControlObject).

Using the cot implementation breaks all midi branch learning / debugging mode code, so I merged in the p->setValue...
This bug is to note this fact and encourage a look at this to determine if this behaviour is safe, and if not to replace it with something safe.

Index: src/midiobject.cpp
--- src/midiobject.cpp (revision 2398)
+++ src/midiobject.cpp (working copy)
@@ -204,10 +221,7 @@

   // I'm not sure entirely why buttons should be special here or what the difference is - Adam
         if (((ConfigValueMidi *)c->val)->midioption == MIDI_OPT_BUTTON || ((ConfigValueMidi *)c->val)->midioption == MIDI_OPT_SWITCH) {
- ControlObjectThread cot(p);
- cot.slotSet(newValue);
- //p->setValueFromThread(newValue);
+ p->set(newValue);
             // qDebug() << "New Control Value: " << newValue << " (skipping queueFromMidi call)";

Changed in mixxx:
assignee: nobody → psyc0de
importance: Undecided → Critical

Might the new MidiMapping class have nullified this bug?

RJ Ryan (rryan) wrote :

If the code is still there it is dangerous. Only the creator/parent of a ControlObject can safely call set() directly.

Albert Santoni (gamegod) on 2009-01-31
Changed in mixxx:
assignee: psyc0de → gamegod
milestone: none → 1.6.2
status: New → Fix Committed
Changed in mixxx:
milestone: 1.7.0-moving → none
ironstorm (ironstorm-gmail) wrote :

This was fixed by Albert (removing the special case for buttons) and then myself (implementing proper conversion of Button up/down to NOTE_ON/OFF) in trunk shortly before the branch for 1.7.0 . It won't be fixed in any release of 1.6.x.

Changed in mixxx:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers