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

[mixxxbot]

Reported by: deftdawg
Date: 2008-11-27T03:41:10Z
Status: Fix Released
Importance: Critical
Launchpad Issue: lp302684
Tags: midi

---

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;
         }

[mixxxbot]

Commented by: Pegasus-RPG
Date: 2009-01-31T02:24:13Z

---

Might the new MidiMapping class have nullified this bug?

[mixxxbot]

Commented by: rryan
Date: 2009-01-31T02:43:55Z

---

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

[mixxxbot]

Commented by: deftdawg
Date: 2009-03-31T14:53:30Z

---

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.

[mixxxbot]

Issue closed with status Fix Released.