Saint's Log

Not using underscores to start #defines

Bug 492625 was a rather easy fix, but one from which I tried to gleam as much as I could from nonetheless. Some /storage/* header files used defines with leading underscores. I used the following regular expression in the search and replace:

Find: (\s+)(_+)(moz\w.+_h)(_+)
Replace With: \1\3

What was interesting was that MXR suggested that there were 72 changes to be made, but the above regex only caught 63. It was only after doing all the manual labor of  combing through the list that I realized that I could have simply done a regex search for lines starting with #define (duh) to determine the source of the discrepancy. It turns out that there were some #defines without the leading underscore but having a trailing one.

It was also interesting trying to find the document defining the C++ standard. I ended up at the JTC1/SC22/WG21 – The C++ Standards Committee page which had the relevant text in the “17.6.3.3.2 Global names” section. Stack overflow was the key pointer as to exactly where the relevant info was:

Certain sets of names and function signatures are always reserved to the implementation:

• Each name that contains a double underscore _ _ or begins with an underscore followed by an uppercase letter (2.12) is reserved to the implementation for any use.
• Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.

Building Firefox With Visual C++ Express 2010

I just installed Visual C++ Express 2010 (the most recent version online) in order to build Firefox. However, MozillaBuild has not yet been officially updated to handle Visual C++ 2010. Only 2008 is currently supported as per the build prerequisites. Thankfully, they mention that the trunk version of MozillaBuild should support VC++ Express 2010. The only thing that wasn’t so obvious here (for someone dashing around trying to get building ASAP) was where on earth to look for this. After some totally unnecessary and mostly unhelpful Google-ing around, I ended up at http://hg.mozilla.org/mozilla-build/rev/ad3bd5686474. The necessary start-msvc10.bat and guess-msvc.bat files did the trick.

Bookmark Service getItemGUID Issues

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.

JSON.stringify now correctly handles spaces

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.

Trivial crash fix in Statement::BindParameters

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:

1. Mozilla Storage Reference
2. XPCOM Macros

MathML mfenced whitespaces broke my patch?

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.

Updated about:plugins page title

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).

“View Source” broken on trunk

A bug of interest for me this week was bug 552636. Here’s how I went about trying finding out what was broken:

1. 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 :).
2. 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.
3. 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).
4. 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.
5. 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.