Contract weekly report #54

HaikuWebKit 1.4.7 was released, which fixes several issues, but also introduce new ones (it seems redirects are not always working). I’ve been spending most of my time in the last month on issues in Haiku, and this HaikuWebKit release is not as well tested as the previous ones. It would be great to have more people look at HaikuWebKit development, as other devs would probably find and fix my bugs much faster than I can (it’s hard to fix your own code).

A new contributor to Haiku: I helped dsizzle refine a set of patches for CharacterMap which are now merged. They improve several things to make the application more useful (hrev48376, hrev48387, hrev48389)

Fixed a bug in ActivityMonitor which would happen sometimes when adding a graph. This one was interesting to track and deserves some explanations. What happened is when adding a new graph, the ActivityMonitor window would freeze. This didn’t always happen, but was reproducible after adding enough graphs. I started to debug the issue and noticed that ActivityMonitor was locked trying to grow the storage for a BShape. BShape is an object used to describe a complex shape for drawing, it is a list of pen movements (move, draw line, and draw bezier curve), and can be either stroked (to draw the shape outline) or filled (to draw the inside). The drawing operations are stored in an array which is grown using realloc when it’s full.

The ActivityMonitor code used a BShape and filled it with LineTo calls to draw the graph contents. This is not the optimal solution, as when drawing only straight line there is a faster way. You can use BView functions BeginLineArray, AddLine and EndLineArray. BeginLineArray takes the number of lines as a parameter, so the memory for the whole operation can be allocated upfront and there is no need to grow the buffer (realloc is an expensive operation, as it is often not able to grow the buffer directly, and must instead allocate a new bigger one, copy the data there, then free the original buffer). So I modified ActivityMonitor to use a line array instead of a BShape. And… instead of freezing, the app started to exit immediately by calling abort(). This is a bit unfriendly as this exits the app directly without the usual debug_server dialog allowing you to attach a Debugger to the app before it’s gone. Some debugging later, I found that BeginLineArray was throwing a C++ bad_alloc exception. These happen when you try to allocate an object (with new) and there is not enough memory for it. The BeAPI doesn’t use exceptions in most cases, instead you are expected to use InitCheck to make sure your objects initialized properly, or check the status_t returned by some methods. In the case of BeginLineArray, however, the BeAPI has no way to report the error, so for this exceptional case we used an exception.

After adding a try/catch block around my BeginLineArray call, I could get past the problem. But not for long. The failed BeginLineArray would let the BView in an inconsistent state after the exception was raised, so an attempt to call EndLineArray would complain that BeginLineArray had not been called, and a new call to BeginLineArray would complain that EndLineArray was not called for the previous array. No way to get out of that. I modified the code a bit to leave the object in a coherent state (with a line array of size 0) when BeginLineArray failed to allocate the memory. Now EndLineArray can be called to clear the error and then operations can resume as usual. So the exception can be caught, the drawing skipped, and then the next time it will work.

But I was still worried about this exception. It happened at times where the memory use of ActivityMonitor was still reasonably low (about 700MB, far from the 2GB address space limit), and the line array itself should be small (about a hundred of points), so there were no obvious reason for the allocation to fail. So I added some tracing to ActivityMonitor to see what the size of line arrays was. Most of the times it was ok, but in some occasions when adding a new graph it would try to allocate an huge one, which is the root cause of the failure. It turns out the line array size depends on the view width, and in some case ActivityMonitor would try to draw a view before it was fully layouted. In that case the view had a very small width, and ActivityMonitor substracts 10 from the width to have a margin on the right of the graph. This would lead to a negative number which was unfortunately stored in an unsigned variable. It overflowed to an huge value, which we would feed to the unsuspecting BeginLineArray call, or, before the changes, use as a loop bound when calling BShape::LineTo. The fix for this was very simple: check for a view width less than 10 pixels and don’t draw anything. Problem solved with a two lines fix!

I also spent some time triaging tickets in the bugtracker, we are now approaching the mark of “2500 tickets to R1”.

I received the package with a new USB gamepad (which works just fine) and USB CD drive. The latter does not work that great. I can use it to mount plain CDs, but CDDA and multi-session won’t work over USB currently. The USB mass storage driver does not use the standard SCSI commands handler as the other storage drivers do, because a lot of USB devices are known to fail in unrecoverable ways when sent SCSI commands they don’t handle. But for CDs, there are some commands we need working (like the “read table of contents” one). It is difficult to make sure we don’t confuse a device by sending it an unknown command without having to grow either a blacklist or a whitelist of devices. I’m not sure I want to start this work just now, but we’ll have to get it done at some point.