Thursday, August 14, 2014

A PropertyGrid gotcha

In a nutshell, when placed in a PropertyGrid, objects that use the same DisplayNameAttribute in different categories lead to surprising selection behavior.

I decline to call it a bug because I suspect the authors of the framework would claim this is how it's supposed to behave, but it's still the sort of thing that can trip developers up. Imagine you set a PropertyGrid's SelectedObject to an instance of this class:

class Demo
{
    [Category("Category1")]
    [DisplayName("Value")]
    public int Val1 { get; set; }

    [Category("Category2")]
    [DisplayName("Value")]
    public int Val2 { get; set; }

    [Category("Category3")]
    [DisplayName("Value")]
    public int Val3 { get; set; }
}

Sure, we're reusing the display name "Value," but since each instance is in a different category, everything should be fine, right? Well; not exactly. Under the hood, the grid determines equality between GridEntrys by comparing the combination of label, property type, and tree depth. And in this case, "label" means display name. So it doesn't matter if the actual code names of the underlying properties differ—as long as their display names, types, and depths are the same, the grid will consider their entries equal. One consequence of this is that if PropertyGrid.Refresh() is called while the Category3 Value is selected, the selection will move to the Category1 Value; presumably because it is the first entry in the tree where .Equals(oldSelection) returns true.

Luckily, there is a workaround. PropertyGrid omits tab characters when it prints display names, so you can simply add a different number of \t's to the beginning or end of each one to make them unique. A bit of a hack to be sure, but it does the job.

Thanks to stackoverflow for assisting me in my investigation of this issue.

Monday, August 11, 2014

A make_shared caveat

When you allocate a new object for a shared_ptr using the obvious approach:

std::shared_ptr<Foo> p(new Foo(a1, a2)); // case 1

you wind up with (at least) two dynamic memory allocations: one for the Foo object itself, and another for the shared_ptr's control block (which includes the strong and weak reference counts).

That's not usually a big deal, but it would help locality of reference if we could put the whole thing into one block, as well as making construction slightly faster. The make_shared library function can help us with that:

std::shared_ptr<Foo> p = std::make_shared<Foo>(a1, a2); // case 2

make_shared will typically place the shared_ptr control block and the allocated object itself into a single chunk of memory. I say "typically" because the standard does not require implementers to do things this way, but in practice, they do.

This seems like win-win, but there is a catch. Imagine a situation where the Foo object we created above is referenced by one shared_ptr and two weak_ptrs (i.e. its strong count is one and its weak count is two). Now that one shared_ptr goes away. If we'd allocated the Foo as in case 1, at this point we'd be able to free all the memory it used to occupy and go on our way. But if we'd done things as in case 2, this isn't possible. Why? Because we still need to keep the control block around until the weak count goes to zero, the control block and the Foo share a single allocation, and it isn't possible to free just part of an allocation. So while the Foo's destructor will be called the moment the strong count goes to zero as expected, if it was created using make_shared, its memory can't be returned to the system until the weak count does too. In situations where dangling weak_ptrs can hang around more or less indefinitely, this can lead to some surprising memory leaks.

Friday, August 1, 2014

pimpl makes move constructors easy

I described the pimpl idiom and some of its uses in a previous post. Another little fringe benefit is that it makes writing move constructors really, really easy. Consider this (partial) class:

class TuneResult
{
private:
    std::string m_name;
    std::map<std::pair<double, double>, double> m_measurements;
    std::pair<double, double> m_optimum;
    std::vector<std::string> m_logMessages;
};

Without pimpl, adding a move constructor requires a bit of effort:

class TuneResult
{
    TuneResult(TuneResult&& other)
        : m_name(std::move(other.m_name))
        , m_measurements(std::move(other.m_measurements))
        , m_optimum(std::move(other.m_optimum))
        , m_logMessages(std::move(other.m_logMessages))
    {    
    }

private:
    std::string m_name;
    std::map<std::pair<double, double>, double> m_measurements;
    std::pair<double, double> m_optimum;
    std::vector<std::string> m_logMessages;
};

It's not hard to imagine that becoming much longer for classes with more members. But if you've already pimpled anyway, moving becomes trivial:

// h file

class TuneResultImpl;

class TuneResult
{
    TuneResult(TuneResult&& other);
    
private:
    friend class TuneResultImpl;
    std::unique_ptr<TuneResultImpl> m_impl;
};

// cpp file 

class TuneResultImpl
{
    friend class TuneResult;

private:
    TuneResult* m_self;

    std::string m_name;
    std::map<std::pair<double, double>, double> m_measurements;
    std::pair<double, double> m_optimum;
    std::vector<std::string> m_logMessages;
};

TuneResult::TuneResult(TuneResult&& other)
    : m_impl(std::move(other.m_impl))
{
    m_impl->m_self = this; // don't forget!
}

Thursday, June 26, 2014

C++: Tying object lifetimes together with shared_ptr and custom deleters

Once upon a time, when a great man died his slaves would be killed and buried alongside him as gifts to carry into the netherworld. Our civilizations have mostly moved beyond both slavery and grave gifts, but our code has not—it is actually quite common to have a series of subordinate helper objects that you want to keep around for exactly as long as some master object is alive. The most straightforward way of doing this, of course, is to have the the slave objects be actual member variables of the master's class. But that won't work if you've made the choice to keep your objects loosely coupled (for any of several excellent reasons). For example, consider a class that allows builders to attach custom behaviors to it using the observer pattern:

class Master
{
public:
   struct IObserver { /* ... */ };

   // WEAKLY attach an observer that will be notified when this object changes
   void AddChangedObserver(std::weak_ptr<IObserver> observer);
};

// write the changes to disk
class Serializer : public Master::IObserver { /* ... */ };

// update UI to reflect changes to the object
class UiUpdater : public Master::IObserver { /* ... */ };

std::shared_ptr<Master> MakeMaster()
{
   auto master = std::make_shared<Master>();

   auto serializer = std::make_shared<Serializer>();
   master->AddChangedObserver(serializer);

   auto uiUpdater = std::make_shared<UiUpdater>();
   master->AddChangedObserver(uiUpdater);    

   return master; // danger!
}

There is a problem here. We want serializer and uiUpdater to stick around as long as the returned Master does, but that requires us to keep a shared_ptr to each somewhere. The code is not doing that now, so both will be deleted (and implicitly deregistered) as soon as MakeMaster() returns. This will not cause any undefined behavior, but we won't get the attached behaviors we were expecting from these objects either.

So where do we keep the shared_ptrs to the two observers? We could return them from MakeMaster() along with the main object, but that's just punting to the caller (who shouldn't need to know that we're attaching behaviors anyway). A better solution is to leverage the ability to give shared_ptr a custom deleter. Instead of return master, we can do this:

std::shared_ptr<Master> master2(master.get(),
    [master, serializer, uiUpdater](Master*) mutable
    {
        master.reset();
        serializer.reset();
        uiUpdater.reset();
    });
return master2;

This requires a bit of explanation. First of all, we are creating a new reference count/shared_ptr "family" over the Master object we created earlier. Normally having more than one reference count for the same object would eventually result in a double-deletion disaster, but that won't happen here. The reason is that we're giving this second shared_ptr a custom deleter lambda that doesn't actually delete anything—instead, it simply hangs on to "keepalive" shared_ptrs to the three objects via captures. However, when the master2 family's strong reference count reaches zero and this deleter lambda is called, it will reset* all three of them—it is this action that will result in the actual deletion of the three objects via the ordinary shared_ptr mechanism. Which is exactly what we want.

*Note: It is important to actually reset the captured shared_ptrs in the deleter and not simply rely on them being destroyed when the lambda is. If this wasn't done, weak_ptrs in the master2 family would be able to keep the objects alive.

Thursday, June 5, 2014

A compatible animated ActionBar refresh icon

Many apps put a refresh icon on the ActionBar, and it's nice to have that icon animate while a refresh is in progress. The internet is full of suggestions on how to do this, but most either require third party libraries (e.g. ActionBarSherlock) or are not compatible with older versions of the API. Here is some simple code that will rotate the refresh icon clockwise for API 4+ (disclaimer: I've only tested it down to API 10).

Note: The code assumes you already have ic_action_refresh in res/drawable. You can get the stock Android refresh icon from the Action Bar Icon Pack here.

res/anim/spin_clockwise.xml:
<?xml version="1.0" encoding="utf-8"?>

<rotate xmlns:android="http://schemas.android.com/apk/res/android"
    android:duration="1000"
    android:fromDegrees="0"
    android:interpolator="@android:anim/linear_interpolator"
    android:pivotX="50%"
    android:pivotY="50%"
    android:toDegrees="360" />

res/layout/refresh_action_view.xml:
<?xml version="1.0" encoding="utf-8"?>

<ImageView xmlns:android="http://schemas.android.com/apk/res/android"
    android:contentDescription="Refresh"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:src="@drawable/ic_action_refresh"
    android:paddingLeft="12dp"
    android:paddingRight="12dp"
    />

AnimatingRefreshButtonManager.java:
import android.content.Context;
import android.support.v4.view.MenuItemCompat;
import android.view.MenuItem;
import android.view.View;
import android.view.animation.Animation;
import android.view.animation.AnimationUtils;

public class AnimatingRefreshButtonManager
{
    private final MenuItem mRefreshItem;
    private final Animation mRotationAnimation;

    private boolean mIsRefreshInProgress = false;

    public AnimatingRefreshButtonManager(Context context,
       MenuItem refreshItem)
    {
        // null checks omitted for brevity

        mRefreshItem = refreshItem;
        mRotationAnimation = AnimationUtils.loadAnimation(
            context, R.anim.spin_clockwise);

        mRotationAnimation.setAnimationListener(
            new Animation.AnimationListener()
            {
                @Override public void onAnimationStart(Animation animation) {}

                @Override public void onAnimationEnd(Animation animation) {}

                @Override public void onAnimationRepeat(Animation animation)
                {
                    // If a refresh is not in progress, stop the animation
                    // once it reaches the end of a full cycle
                    if (!mIsRefreshInProgress)
                        stopAnimation();
                }
            }
        );

        mRotationAnimation.setRepeatCount(Animation.INFINITE);
    }

    public void onRefreshBeginning()
    {
        if (mIsRefreshInProgress)
            return;
        mIsRefreshInProgress = true;

        stopAnimation();
        MenuItemCompat.setActionView(mRefreshItem,
            R.layout.refresh_action_view);
        View actionView = MenuItemCompat.getActionView(mRefreshItem);
        if (actionView != null)
            actionView.startAnimation(mRotationAnimation);
    }

    public void onRefreshComplete()
    {
        mIsRefreshInProgress = false;
    }

    private void stopAnimation()
    {
        View actionView = MenuItemCompat.getActionView(mRefreshItem);
        if (actionView == null)
            return;
        actionView.clearAnimation();
        MenuItemCompat.setActionView(mRefreshItem, null);
    }
}

Skeleton Activity demonstrating usage:
import android.app.Activity;
import android.view.Menu;
import android.view.MenuItem;

public class SomeActivity extends Activity
{
    private AnimatingRefreshButtonManager mRefreshButtonManager;
    
    @Override public boolean onCreateOptionsMenu(Menu menu)
    {
        getMenuInflater().inflate(R.menu.main, menu);

        MenuItem refreshItem = menu.findItem(R.id.action_refresh);

        mRefreshButtonManager =
           new AnimatingRefreshButtonManager(this, refreshItem);

        return super.onCreateOptionsMenu(menu);
    }
    
    @Override public boolean onOptionsItemSelected(MenuItem item)
    {
        switch (item.getItemId())
        {
            case R.id.action_refresh:
                mRefreshButtonManager.onRefreshBeginning();
                // start the refresh
                return true;
        }

        return super.onOptionsItemSelected(item);
    }
    
    // how you detect this is application-specific
    void onRefreshComplete()
    {
        mRefreshButtonManager.onRefreshComplete();
    }
}

Monday, May 19, 2014

Android: Stop your ringtones before sending them to the garbage collector

For some time, I've been aware that the warning "MediaPlayer finalized without being released" was occasionally showing up in my app's logcat output. This was a bit mystifying since I don't use MediaPlayer anywhere, but there were more important things to work on.

Today I finally got fed up and tried to figure out what was going on. It turns out that ringtones are the culprit. RingtoneManager.getRingtone() calls open() on the Ringtone object it returns. Ringtone.open() sets up a MediaPlayer. And this MediaPlayer is only released if you call Ringtone.stop() before letting the thing get GC'ed. Note that you do not actually need to play the ringtone for this to be true. As far as I can tell, none of this is documented.

Anyway, I have no idea how much harm there is in neglecting to stop your ringtones and release their media players, but doing so will at least get rid of a warning message.

Monday, May 12, 2014

A trick for tracking down tough memory leaks in VC++

Memory leaks are a common problem in C++. Even if you use smart pointers everywhere, you can still leak memory if you create a shared_ptr cycle. If this happens in a debug build and you've enabled leak detection as described here, Visual Studio will alert you in the output at shutdown:
Detected memory leaks!
Dumping objects ->
C:\PROGRAM FILES\VISUAL STUDIO\MyProjects\leaktest\leaktest.cpp(20) : {10021} 
normal block at 0x00780E80, 64 bytes long.
 Data: <                > CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD CD
Object dump complete.



The filename and line number may or may not be present, and may or may not be useful. If they identify one of your own files, and if there aren't too many different paths to the indicated location, they might be all you need to know. But if they point to something in the guts of vector or make_shared, they tell you practically nothing.

That "{10021}," on the other hand, has potential. What it's telling you is that the block returned by dynamic allocation #10021 was never freed. Unfortunately, by this point it's too late to say what allocation #10021 was. You can run the program again and use this technique to break when it occurs, but if there's any indeterminacy in what you allocate and when, it's not going to be the same call stack. In particular, if your application is multi-threaded (and what applications aren't anymore), none of the allocation numbers after the second thread spawns will be stable from one run to the next.

The third-party tool Visual Leak Detector is designed to help you in these situations, but (at least in my experience) it doesn't always give the right answer. So here is a sort of brute-force approach that can be used when all else fails.

The brute-force leak detector


(tested with VS2008-2012; things may be different in 2013)
  1. Get debug symbol info for the VC++ runtime if you don't have it already. The quickest way is Tools > Options > Debugging > Symbols > check "Microsoft Symbol Servers."
  2. Comment out code that spawns nonessential threads if they aren't part of the problem, and do anything else you can think of to temporarily reduce concurrency. The goal is to get to a place where the leaked allocation's number, while not completely stable, is at least restricted to a known, reasonably narrow range.
  3. Set a breakpoint in dbgheap.c at the top of _heap_alloc_dbg_impl(). If intellisense is working, you can get to this file by typing _crtBreakAlloc somewhere in your code and doing a "Go To Definition" on it. Otherwise check "[drive]\Program Files (x86)\Microsoft Visual Studio [version]\VC\crt\src."
  4. Right-click the breakpoint dot and choose "Condition..." Make the condition "_lRequestCurr >= [minimum] && _lRequestCurr <= [maximum]," where "[minimum]" and "[maximum]" are the endpoints for the range you observed in step #1.
  5. Right-click the breakpoint dot again and select "When Hit...". Choose "Print a message" and make that message "***{_lRequestCurr} : $CALLSTACK". Make sure "Continue execution"  is checked.
  6. Start the program. It will run much more slowly than usual due to the need to evaluate the tracepoint each time the code passes over it. If you already have the leak narrowed down to a particular area of the code, it will help to disable the tracepoint until the suspect code is reached.
  7. Once you're sure you've done whatever it takes to trigger the leak, shut the program down. Note the allocation numbers in Visual Studio's memory leak messages.
Every time a dynamic allocation happens, the tracepoint will dump the allocation number to the output followed by its callstack. So once the program exits and you see a message like the one above indicating that allocation 10021 was leaked, you can simply search the output for "***10021" to find the corresponding callstack. With any luck, that will tell you what you need to know.