Open Bug 474716 Opened 11 years ago Updated 1 year ago

Add ID check test to Firefox

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

People

(Reporter: kairo, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attached patch add ID check test (obsolete) — Splinter Review
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.
Attachment #358092 - Flags: review?(gavin.sharp)
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.
Attachment #358092 - Attachment is obsolete: true
Attachment #368688 - Flags: review?(gavin.sharp)
Attachment #358092 - Flags: review?(gavin.sharp)
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.
Attachment #368688 - Flags: review?(gavin.sharp)
Depends on: 446281
Assignee: kairo → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
No assignee, updating the status.
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.