474716 - Add ID check test to Firefox

Status: NEW

(Firefox :: General, defect)

Description

[Robert Kaiser]

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.

Comment 1

[Robert Kaiser]

Attachment: add ID check test

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

Comment 2

[Serge Gautherie (:sgautherie)]

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 ?

Comment 3

[Robert Kaiser]

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 4

[Robert Kaiser]

Comment on attachment 358092 [details] [diff] [review]
add ID check test

Requesting review. Note comment #1 and comment #3 though.

Comment 5

[Serge Gautherie (:sgautherie)]

Gavin: ping ?

Comment 6

[Robert Kaiser]

Attachment: v 1.1: only main window, prefs commented out

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 7

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.