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

Reported by ironstorm on 2008-11-27
2
Affects Status Importance Assigned to Milestone
Mixxx
Critical
Albert Santoni
1.6
Critical
Unassigned
1.7
Critical
Unassigned

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)";
             return;
         }

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