What Could Possibly go Wrong?

Just on the 2.24 code freeze a memory leak in the trash applet was pointed out to me. The proposed patch was simple:

     }

   g_object_unref (icon);
+  g_object_unref (info);
 }

 static void

It came from a trusted contributor. I checked that the the info variable really did need unrefing. I ran it past the release team to get their approval for a freeze-break. So I applied the patch and sent 2.24.0 out the door.

The one thing I didn’t do was test the applet thoroughly with the patch applied. If you run the trash applet from 2.24.0 and empty the trash from the context menu then it will crash a few seconds later. Fortunately for me, by the time I woke up and got the bug report in my morning email, the team of Pedro Villavicencio, Sebastien Bacher and Matthias Clasen had identified the root cause of the problem and the fix was tested and committed to SVN by lunchtime. (Vincent Untz rolled a 2.24.0.1 just in time for the official GNOME 2.24 release.)

So what went wrong with such a simple patch? The fix for the memory leak looks correct – and it is. The problem is the line above it: g_object_unref (icon);, icon is an object that should not be unrefed. The leaked info object was somehow protecting it from being overwritten once freed and the other code relying on it would go on quite happily. With the fix applied, the icon object was being overwritten and a segfault occurred the next time it was accessed.

There are two morals to this story: The first is that there is no such thing as a trivial patch. The second is that testing is essential: either by doing it in an automated way or by farming it out to all the people prepared to run beta versions. There are various stupid reasons why applets are hard to test, something I intend addressing in the coming release cycle, but my most immediate resolution is to respect the code freeze for everything that isn’t actually causing a crash (I fully expect to have forgotten this in six months time).


Posted

in

by

Tags: