We are often asked whether we send bug reports to developers of open-source projects. The answer is yes. More than that— we sometimes track the progress. This article is about one of the cases when this habit prevented a sloppy bug fix.
We all know that it's important to report bugs. Everybody likes when software works quickly, correctly, and stably. However, not everyone checks in on their bug report after sending one. But if you participate a bit more, you can hasten the bugfix or even help fix more than originally intended.
The point being — a couple of months ago we published an article with the Chromium project check results. Then I sent a bug report to the developers. But it did not go as smoothly as I wanted it to – otherwise I wouldn't have needed to write this note. So what went wrong?
Disclaimer:
everybody makes mistakes. I highly respect Chromium developers and the work they do. It was just an interesting case I used as an example :)
I appreciate how quickly the developers fix the bugs detected. Despite the huge list of open issues, they processed my report the same day, and even committed a fix. Sometimes it happens differently.
Before we start, let's look at this error once again (case N8 from the original article):
V501 There are identical sub-expressions 'file.MatchesExtension(L".xlsb")' to the left and to the right of the '||' operator. download_type_util.cc 60
ClientDownloadRequest::DownloadType GetDownloadType(const base::FilePath& file)
{
....
if (file.MatchesExtension(FILE_PATH_LITERAL(".apk")))
return ClientDownloadRequest::ANDROID_APK;
....
else if (file.MatchesExtension(FILE_PATH_LITERAL(".pdf")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".doc")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".docx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".docm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".docb")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".dot")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".dotm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".dotx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xls")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) || // <=
file.MatchesExtension(FILE_PATH_LITERAL(".xlt")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xlm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xlsx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xltx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xltm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) || // <=
file.MatchesExtension(FILE_PATH_LITERAL(".xla")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xlam")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xll")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xlw")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".ppt")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".pot")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".pps")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".pptx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".pptm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".potx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".potm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".ppam")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".ppsx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".ppsm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".sldx")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
file.MatchesExtension(FILE_PATH_LITERAL(".rtf")))
return ClientDownloadRequest::DOCUMENT;
....
}When I checked my e-mail, I saw a notification that the developers made a commit for my bug report. Hm... In just one day? My curiosity made me look what was going on. And for good reason...
When describing one of the errors in that article, I covered the problem partly and left the rest for developers to figure out for themselves. My teammates had warned me about this situation and problems it might cause. You can see the result above — the fix that the developers applied is highlighted in red. Note the items highlighted in yellow – these are duplicate values the developers needed to find additionally.
Looks like the developers quickly fixed the code and had no extra time to delve into the article. Well, I need to take this into account next time...
Of course, I immediately contacted the developers and pointed out the additional value to check/remove.
Well, now the bug is correctly fixed correctly and we can discuss whether it was possible to avoid this problem at all.
On the one hand, Chromium developers have enough work already. Carefully reading other people's articles to find additional problems is probably not the thing they get paid for. On the other hand, the code quality suffers. It's really hard to notice an error in the example above, even if you know it's definitely there. Oh, if only there was a way to catch these errors... Wait, I know one!
I'm not sure about the classic code review (after all, the code got into the repository), but most static analyzers would have found this error. At least, they should, because that's exactly the point of static code analysis – to search for errors in newly written or modified code.
Someone might say that you just need to pay more attention and structure the code well. The advice is good, but, unfortunately, in real projects it is not always possible. Maybe I didn't mention some other options... Let's discuss them in the comments!
By the way, we already had a similar case — with the CovidSim project check. There, the developers also didn't have enough time or resources to fix the code. You can read about this and another similar case in my teammate's articles: "How PVS-Studio prevents rash code changes" and "How PVS-Studio prevents rash code changes, example N2".
Do you track your bug reports to see what happens to them? How does this usually go for you? Please leave a comment below!
