String assignment not threadsafe



I’ve been investigating an intermittent crash today and it seems to come down to the String class assignment operator not being threadsafe:

const String& String::operator= (const String& other) throw()
if (this != &other)
Atomic::increment (other.text->refCount);

    if (Atomic::decrementAndReturn (text->refCount) == 0)
        juce_free (text);

    text = other.text;

return *this;


My scenario is fairly simple:
non-message thread is assigning different status messages to a String and calling triggerAsyncUpdate (a lot);
message thread is assigning the status message String to a label.

By the way, I was being careful and using a critical section, but got caught out with a const String& returning outside of the critical section. Once I replaced this with a const String return everything seems OK.

What seemed to be happening was that both threads managed to call String::operator= at the same time. The first one decrements refCount to zero and calls free. Then, before it gets a chance to execute text = other.text, the second thread increments the ref count to one and copies the free’d text. Next time round the text is free’d for a second time, causing the crash.

Obviously, using a Critical Section and being very careful with const String& solves the problem, but I wonder if anyone else has any thoughts on this.


Wow - that’s subtle! Thanks for spotting it, I clearly need to revisit the string and double-check things like that. I’ll do it asap.


I just found out that my problems with heap corruption were due to this bug. Is there a quick’n’dirty fix for this? I have tried exchanging String.cpp/.h but that does not work so easily. I want to keep JUCE146, because it would require too much code changes (days of work) to adapt to the new JUCE.


I think I had to create a new “exchange” atomic op to avoid this problem, so it’d be pretty hard to back-port. Especially since the whole string class and atomic ops design has also changed since then!


Just wanna ask, do you think this is enough to solve the problem (seems to work here). I’ve added a lock that prevents the Labels accessing the statusMessages while they are being changed:

[code]// called from another thread!
void ThreadWithProgressWindowEx::setStatusMessage(int n, const String &message)
ScopedLock lock(statusMessageLock);

	if (n==0) statusMessage=message;
	else statusMessage2=message;


// called on GUI thread
void ThreadWithProgressWindowEx::changeListenerCallback(void *obj)
if (obj==this)
ScopedLock lock(statusMessageLock);
label->setText(statusMessage, false);
label2->setText(statusMessage2, false);


I wouldn’t do it that way - looks a bit risky for deadlocks between your lock and the messagemanagerlock. Better to use a messagemanagerlock around the place where you set the text.


Are you sure?

  1. You did it the +/- same way as my code in ThreadWithProgressWindow::setStatusMessage()
  2. We all now that MessageManagerLock was faulty in JUCE146, so I’m trying never to use it

Maybe if you explain me how a deadlock could arise, I’d understand.


Ah yes, I’ve even changed the MM lock since then. Actually, looking at it again, you’re probably ok with that locking pattern.