String assignment not threadsafe

General JUCE discussion
1

Hi,

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.

2

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.

3

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.

4

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!

5

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;
}
sendChangeMessage(this);

}

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

6

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.

7

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.

8

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