Bug 484026 from last November was an interesting peek into the Firefox bookmarks code. I looked into Dan Mill’s claim that getItemGUID was creating new GUID annotations for deleted items. I started off by creating an xpcshell test for the bug. Interestingly, the test passed! The problem had apparently been fixed at some point.
My first thought on how to proceed was that perhaps the error was being thrown from the wrong method, hence my erroneous attempt to patch nsNavBookmarks.cpp (probably an instance of “if it isn’t broken…”). That would explain why Marco Bonardo didn’t think the change was such a great idea – i.e. you might as well go changing every other possible caller if you aren’t fixing the source.
Nonetheless, it was interesting to note the use of scopers and how declaring this mozStorageStatementScoper eliminated the need for if (statement) statement->reset() calls at every function exit point! I’m sure the idea goes way back, but whoever figured out that a scoping object’s destructor could handle this is a genius in my humble (C++ wise) opinion.
So, even though I didn’t get some C++ action as I had anticipated, I still picked up a thing or two – such as the coding standard used for the bookmarks service xpcshell tests, and how to use the do_throw and do_check_eq functions.
Bug 505228 (from Sep 2009) is now checked in. The patch fixes a problem with the TraceMonkey JSON encoder where the nesting of arrays is correct for all but the first line of the output string when the “space” argument is specified. See the ECMAScript wiki for more information on JSON support. My initial patch simply (and naively) called the WriteIndent function. I didn’t catch this mistake until I wrote an automated test for the bug. This was the first bug for which I wrote an xpcshell test – it’s amazing how difficult it was for me to figure out how to run these (rather embarrassing in retrospect, but oh well, at least I stayed the course). The JSON xpcshell tests can be run using:
cd ~/mozilla/obj
make -C dom/src/json/test/ xpcshell-tests
To check just the specific file in question (the one to which I added the test), I used
make SOLO_FILE="test_replacer.js" -C dom/src/json/test/ check-one
This was helpful since I could run a smaller subset of tests while developing, and then later run the whole JSON test suite.
Just noticed bug 555087 today, and what better way to usher in the evening than combing through macro definitions? It’s obvious from the crash report that dereferencing array is the cause of the crash in the block:
BindingParamsArray *array = static_cast(aParameters);
if (array->getOwner() != this)
return NS_ERROR_UNEXPECTED;.
Shawn Wilsher suggests tossing in the ubiquitous (although not ubiquitous enough it seems) NS_ENSURE_ARG macro to ensure array is a valid pointer. NS_ENSURE_ARG_POINTER (which maps to NS_ERROR_INVALID_POINTER ) seems more appropriate though from those definitions. One hg diff command later and then the only other question in the galaxy that matters… how on earth <insert odd reference to galaxy> do you create a crash test for this bug that’s aptly named “crash [@mozilla:: storage:: Statement:: BindParameters(mozIStorageBindingParamsArray*)]”?
Quick reads:
- Mozilla Storage Reference
- XPCOM Macros