566351 - Fix context menu test leaks

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

cfx -a firefox -b /Applications/Minefield.app/ -F context-menu test:

Tracked memory objects in testing sandbox: 2
warning: LEAK resource://jetpack-core-jetpack-core-lib/memory.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/unload.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/unit-test.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/timer.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/xpcom.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/file.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/byte-streams.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/text-streams.js
warning: LEAK resource://jetpack-core-jetpack-core-tests/test-context-menu.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/context-menu.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/xul-app.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/api-utils.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/collection.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/window-utils.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/errors.js
warning: LEAK [object Object]

Minimal test:

  exports.testLeaks = function (test) {
    Cc["@mozilla.org/appshell/window-mediator;1"].
      getService(Ci.nsIWindowMediator).
      getMostRecentWindow("navigator:browser").
      content.
      QueryInterface(Ci.nsIInterfaceRequestor);
    test.pass("XXX");
    test.done();
  };

Remove the QueryInterface and the leaks go away.

Comment 1

[Drew Willcoxon :adw]

test.done() shouldn't have been in the test above.  Here's the right test:

  exports.testLeaks = function (test) {
    Cc["@mozilla.org/appshell/window-mediator;1"].
      getService(Ci.nsIWindowMediator).
      getMostRecentWindow("navigator:browser").
      content.
      QueryInterface(Ci.nsIInterfaceRequestor);
    test.pass("XXX");
  };

Another minimal case that's slightly different.  This QI's the browser chrome window and requires a call to sendMouseEvent to leak, while the previous test QI's a content window and that's it.

  exports.testLeaks2 = function (test) {
    Cc["@mozilla.org/appshell/window-mediator;1"].
      getService(Ci.nsIWindowMediator).
      getMostRecentWindow("navigator:browser").
      QueryInterface(Ci.nsIInterfaceRequestor).
      getInterface(Components.interfaces.nsIDOMWindowUtils).
      sendMouseEvent("contextmenu", 0, 0, 2, 1, 0);
    test.pass("XXX");
  };

test.pass() is only needed to keep the tests from being marked as failed and can be removed without affecting the leaks.

Comment 2

[Drew Willcoxon :adw]

To be clear, these minimal tests don't require("context-menu") or anything else outside of the functions.  The only thing in the file are these test functions.

Comment 3

[Drew Willcoxon :adw]

The warnings in comment 0 are those that appear when running the test as it exists in the repo.  With the minimal tests only, the warnings are:

warning: LEAK resource://jetpack-core-jetpack-core-lib/memory.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/unload.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/unit-test.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/timer.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/xpcom.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/file.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/byte-streams.js
warning: LEAK resource://jetpack-core-jetpack-core-lib/text-streams.js
warning: LEAK resource://jetpack-core-jetpack-core-tests/test-context-menu.js
warning: LEAK [object Object]

Comment 4

[Drew Willcoxon :adw]

Attachment: open a new tab for each test

I poked around the QueryInterface call and the xpconnect boundary in gdb, but I have no idea what's going on.  It's as if the QI causes the content window to take ownership of the global JS context in the sandbox that the test harness creates and runs the test inside.  Creating a new browser chrome window, running the test, and then closing the window stops the leaks.  So does creating a new tab (and therefore a new content window) and closing it when the test is done, and that's what this patch does.

Comment 5

[Atul Varma [:atul]]

Comment on attachment 445994 [details] [diff] [review]
open a new tab for each test

Nice fix, thanks for tracking this down. Works great--now we just need to talk to the platform team and figure out what's making these leaks occur...

Comment 6

[Drew Willcoxon :adw]

Thanks for the quick review.  Yeah, I'll file a bug in Core or JS or wherever about this, but first I'd like to get a minimal test case that's independent of Jetpack, so I'll work on that.

Pushed: http://hg.mozilla.org/labs/jetpack-sdk/rev/0f90ee13d1d6

Comment 8

[Atul Varma [:atul]]

Hey Drew, did you file a bug about this in core/js? Myk is having memory leak problems with panel-related code and I have a sneaky suspicion that it's related to this kind of bug.

Comment 9

[Drew Willcoxon :adw]

No, because I don't know what the bug should say.  I started paring down the entire Jetpack runtime with the minimal test case above but got distracted with other things and never finished.

See also bug 574389, where simply removing a test surfaced a memory leak.

Comment 10

[Atul Varma [:atul]]

No worries! Just filed bug 592180, which is based off the example in comment 1. Hopefully some platform folks can help us get to the bottom of this now.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".