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
Bug 537916 would easily be one of the easiest bugs to fix in the entire code base (typos aside) – the problem in question being that white space is not being stripped out of the <mfenced>’s “separators” attribute. It was exciting (for an XPCOM-string newbie I guess) combing through the string functions to find the right tool for this very crucial bug blocking the next version of Firefox ;).
It wasn’t too long before I stumbled upon FindChar(). Unfortunately, in my rather foolish quest to get some code written as soon as possible, I stopped looking through the function list and put together a possible solution (that I even considered a marvel of beauty for just but a second).
The grin of janitorial success had hardly faded from my face when Frédéric Wang pointed out the rather obvious StripWhitespace() method! I guess it’s always a good idea to look through the whole function list! Alright, so it still remains one of the easiest fixes even in light of the fact that someone else had to point out the most obvious solution.
Next, Roc wants a test for this! How on earth do you create a text file with the right spaces, i.e. one containing a portion where each blank is a whitespace sequence “U+0020 U+0009 U+000a U+000d”? I came up with sudo apt-get install hexedit; hexedit layout/reftests/mathml/mfenced-1.xhtml but my guess is that there is a better way to do this (like vi)?
Minor as this bug is, I would rank it is as one of the more interesting ones I have worked based on how much it freaked me out – I’ll be the first to confess that the resultant patch blew my mind. It “looked” wrong and I could have sworn it was broken/invalid because I hadn’t encountered newlines as data in unified context diff patches before. I guess you do learn something new each day – even about newlines after having used computers for over 15 years of your life. Thankfully, I now also know how Hg determines which the binary files are.
From my mozilla endeavours last week comes a 2 line update to the page title on mozilla’s about:plugins page. See bug 548481 for the patch. We’re now calling it the “Enabled Plugins” page since that’s what it actually shows (see bug 180568 for details).
A bug of interest for me this week was bug 552636. Here’s how I went about trying finding out what was broken:
- I examined the infamous bug 469302 which Stappel claimed was back. From its patch, it was easy to see that viewSource.js could easily be the culprit yet again. This was my first look at viewSource.js :).
- Next, I looked its Hg annotations. The only changeset that looked suspicious to me was dao@33423. It looked like Dao had replaced getBrowser().webNavigation.sessionHistory with gBrowser.sessionHistory which seemed incorrect since he had replaced other instances of getBrowser().webNavigation with getWebNavigation(). So my idea was to switch this to getWebNavigation().sessionHistory. I made my changes, reran make, and voila!! Nothing changed, bug still present, moon still shining.
- My next thought was that perhaps I could get some insight from the bug Dao was working on that created this changeset (it gets me every time that the changeset hash is a link but has no special formatting to make it stand out on this page). This led me to bug 517755 and consequently, to the MDC browser and nsIWebNavigation – MDC pages. It wasn’t long afterwards that I concluded that this changeset most likely wasn’t the culprit (and that I needed to learn what the heck smart getters are all about).
- As a PHP developer, print_r and logging usually comes in handy every once in a while. Being new to Firefox chrome code however, I had no idea how to capture some debugging info (which is probably why I went around digging in changesets after finding the possible approximate bug location instead of getting some helpful information from the running code). I tried removing the try-catch block – nothing showed up on the console, and inserting lots of services.log(…) lines (or something like that straight out of MXR) around the “suspicious” code – nothing showed up on the console.
- That was when I found Debugging a XULRunner Application – MDC. Perhaps the most important piece of info was to set the javascript.options.showInConsole pref, and in came Components.utils.reportError(typeof(arg)) and Components.utils.reportError(arg). That was when I discovered that typeof(arg) was now “function” and not “object” as viewSource expected, as reflected in the patch I attached to bug 552636. arg itself turned out to be “[object XPCNativeWrapper [xpconnect wrapped nsISupports]]”.
One of the fun things about tracking bugzilla is just how keenly people monitor some of these bugs. It was not long afterwards that Josh Mathews pointed out that the culprit had been bug 553407 all along! Now it’s time to dissect that patch and see exactly what this whole XPCNativeWrapper business is all about.