You don’t know what you’ve got ’till it’s gone – or, tab load detection gone wrong

TL;DR: if you own/wrote code that adds an event listener for the "load" event on a tab, doublecheck that you’re listening for that event on a tab’s <browser> element, and not on the <tab> (eg. the value returned by gBrowser‘s selectedTab property).

In the past two weeks some of us on the Firefox front-end team have been working to get the UX tinderbox builds green again. When we initially started work on Australis, we broke a couple of tests and in the interests of moving forward those were not immediately fixed – we were on a branch, out of the way of everyone else, and it wasn’t our number one priority. Over time this resulted in pretty orange trees, and so we recently moved to fix all the tests we’d broken and return the tree to a functioning, green state.

Although some tests did catch bugs on our side, in general we found subtle bugs in existing tests, which we have (helped) fix both on mozilla-central and the UX branch. One of those that I figured deserved a blogpost of its own was a pattern that tended to look roughly like this:

let tab = targetBrowser.selectedTab;
let win = tab.linkedBrowser.contentWindow;
let expectedReadyState = aURL == "about:blank" ? ["interactive", "complete"] : ["complete"];

if (aOnload) {
  let handler = function() {
    if (tab.linkedBrowser.currentURI.spec != aURL ||
        expectedReadyState.indexOf((win.document || {}).readyState) == -1) {
      return;
    }
    tab.removeEventListener("load", handler, false);
    executeSoon(aOnload);
  }
  tab.addEventListener("load", handler, false);
}

Unfortunately this code broke with the Australis changes, which include removing the empty favicon for pages which don’t have one. The bug in the code may not be immediately obvious, but what the code was doing was adding an event listener on a tabbed browser’s <tab> element (the leaf in the tabstrip at the top), rather than the corresponding <browser> (containing the actual page). Why did that work at all? Well, the tab element contains an image: the page’s favicon. When that loaded, a load event bubbled up. As we’re removing the faviconĀ if the page doesn’t have one, rather than replacing it with an empty placeholder as we do on current versions of Firefox, no such event fires for pages without a favicon, and the relevant tests/code broke completely in Australis (for some of these tests, it is quite possible they caused occasional random orange before).

The manual checking of the page’s readyState is a clue that this isn’t the best way to detect when the page has loaded. Instead of using tab.addEventListener, use tab.linkedBrowser.addEventListener (and the same for removeEventListener, of course).

I’ve attempted to audit the mozilla-central tree for this kind of code, but I may not have been thorough enough by just grepping for things like tab.addEventListener. If you have written code depending on tab loads, be it in tests, or add-ons, or elsewhere, doublecheck how you detect these events!