In bug 473686 comment #10 and later, we concluded that the ID check test we have running for a number of SeaMonkey windows should be added to Firefox as well. I have a patch for that but I still need to test if Minefield leaks something when running it. For now, I have it testing the main browser window and the preferences window.

Not sure who can review this, but here's a patch for adding this test for the main window and the prefwindow. Everything looks well for the main window in this test, but the prefwindow fails the id checks for "bundlePreferences" and "bundleBrand", which seems to be used multiple times, and the prefwindow also leaks on my Linux machine: TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 756 bytes during test execution TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsComponentManagerImpl with size 276 bytes TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 3 instances of nsLocalFile with size 120 bytes each (360 bytes total) TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsPluginHostImpl with size 84 bytes TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total) TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 2 instances of nsTArray_base with size 4 bytes each (8 bytes total) TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsVoidArray with size 4 bytes

The 'Error Console' should pass too, as with SeaMonkey. Could the code be moved in Core or Toolkit (using some #ifdef or whatever), rather than duplicated in each application ?

Making at least parts of the code more common and possibly put it into toolkit is a further step I assume we can do once this can get into the tree. Same for adding more windows to be tested.

Comment on attachment 358092 [details] [diff] [review] add ID check test Requesting review. Note comment #1 and comment #3 though.

Gavin: ping ?

Here's a version that can safely be checked in, as it only tests the non-leaking main window, the code for testing the prefwindow is commented out, someone really should fix the leak and then enable this.

Comment on attachment 368688 [details] [diff] [review] v 1.1: only main window, prefs commented out >diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in >- browser_ctrlTab.js \ What's this about? >diff --git a/browser/base/content/test/test_idcheck.xul b/browser/base/content/test/test_idcheck.xul >+ // Have all loaded windows been closed again? >+ function AllWindowsClosed() >+ { >+ for each (var e in window.gLoadedWindows) >+ return false; >+ return true; How about: return window.gLoadedWindows.__count__ == 0; >+ function DisambiguateCharsetMenulist(aDocument, aListID, aPrefix) >+ { >+ let menulist = aDocument.getElementById(aListID) >+ .getElementsByTagName("menuitem"); >+ for each (let menuitem in menulist) >+ { >+ menuitem.id = aPrefix + menuitem.id; >+ menuitem = menuitem.nextSibling; This looks wrong... doesn't the value you're setting here get clobbered by the loop's assignment? This method is also unused. >+ // now check the ids >+ CheckIDs(aDocument, window.gLoadedWindows[aDocument.location.href]); gLoadedWindows contains arrays of ignorable IDs? That's pretty non-obvious based on the name... perhaps it should contain an object with a "ignorableIDs" property just for clarity. I think I'd prefer to remove the parts that are currently being unused (preferences stuff, some empty methods, etc.) rather than just commenting them out. We can always add them later when those issues are sorted out, and we could put them in their own test files with the common functions in a shared .js file.

No assignee, updating the status.

No assignee, updating the status.

No assignee, updating the status.

No assignee, updating the status.