This test started leaking on fx-team today on Linux debug builds after bug 917226 landed. We then backed that bug out and this tabview test continued to leak. I'm disabling this test on Linux debug for now, and it will need to be fixed and reenabled later.

Disabled in https://hg.mozilla.org/integration/fx-team/rev/0df0f2fe3bbf

The leaks looked like https://tbpl.mozilla.org/php/getParsedLog.php?id=36831629&tree=Fx-Team

I can reproduce this 100% of the time on my Linux VM. It seems to be related to the webNavigation.setCurrentURI() call in content-sessionStore.js - that in turn leads to a leak somewhere in browser.js' onLocationChange handler. Panorama is probably not the right component but I won't move this before I know the real cause.

This seems caused by the BookmarkingUI.onLocationChange() call.

The BookmarkingUI.updateStartState() call seems to cause the leak here somehow.

The statement returned by asyncGetBookmarksId() is canceled but still leaks for some reason. Commenting out the aCallback use in handleCompletion() fixes it.

Even though I should have known better and disabled them all, disabled just browser_tabview_bug628061.js and browser_tabview_bug650280_perwindowpb.js in https://hg.mozilla.org/integration/fx-team/rev/916460a17836 to reopen fx-team after the latest episode of shuffling tabview tests.

(In reply to Tim Taubert [:ttaubert] from comment #7) > The statement returned by asyncGetBookmarksId() is canceled but still leaks > for some reason. Commenting out the aCallback use in handleCompletion() > fixes it. Explicitly nulling aCallback helps too. It would be great to figure out what holds onto the object though.

I had no luck figuring out the cause of the leak. Just passing the handler to AsyncStatementCancelWrapper.executeAsync() makes it somehow leak, even not storing it to _callback still leaks. Nulling out after receiving handleCompletion() however does work so I just went for that.

Try looks good afaict.

Comment on attachment 8399153 [details] [diff] [review] 0001-Bug-989083-Clear-callback-and-scope-variables-in-asy.patch Review of attachment 8399153 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/PlacesUtils.jsm @@ +1349,5 @@ > } > + > + // Clear callback and scope variables as not doing so > + // can for yet unknown reasons cause us to leak those. > + aCallback = aScope = null; we may completely remove aScope, and rather use an arrow function (or bind) in the callers... I can't tell if that is going to make a difference but could be worth a try (test if it's still needed to nullify aCallback). And in case that helps, the method should throw if invoked with more than 2 arguments to check for consumers (there's only one add-on using it btw). I wonder what made this start failing... maybe it's related to ggc? This makes me a little bit nervous.

Comment on attachment 8399153 [details] [diff] [review] 0001-Bug-989083-Clear-callback-and-scope-variables-in-asy.patch Review of attachment 8399153 [details] [diff] [review]: ----------------------------------------------------------------- clearing, see comment 15... I'm not very comfortable to take a workaround like this without figuring the issue, but I'd like at least that experiment to happen. Then if it's still an issue we may even workaround temporarily with a follow-up bug filed.

Removing the aScope parameter doesn't help. Building without GGC still leaks.

We're tickling this leak now over in bug 988313. If this leak blocks that from landing, I think we should strongly consider getting the workaround landed for now as the styleinspector tests are one of our most frequent sources of mochitest-bc orange. Try run of this patch on top of that rewrite: https://tbpl.mozilla.org/?tree=Try&rev=fb5945e8884d

The patch we talked about on Vidyo. Reverting to aCallback() makes us leak again so this patch should be good and fixes everything for me locally.

Comment on attachment 8403285 [details] [diff] [review] 0001-Bug-989083-Stop-leaking-aCallback-and-remove-aScope-.patch, v2 Review of attachment 8403285 [details] [diff] [review]: ----------------------------------------------------------------- you forgot to update the calls in test_PlacesUtils_asyncGetBookmarkIds.js, apart that, looks good.

(In reply to Marco Bonardo [:mak] from comment #21) > you forgot to update the calls in test_PlacesUtils_asyncGetBookmarkIds.js, I did, they didn't really use |this| but I fixed them, thanks! Will land when try comes back green.

Comment on attachment 8403285 [details] [diff] [review] 0001-Bug-989083-Stop-leaking-aCallback-and-remove-aScope-.patch, v2 Looks like we're *possibly* hitting the same leak on Aurora :/ https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=c1fe4d4bfcec [Approval Request Comment] Bug caused by (feature/regressing bug #): No idea. User impact if declined: No user impact, lots of oranges on our CI. Testing completed (on m-c, etc.): Lots of try runs :) Risk to taking this patch (and alternatives if risky): Very low risk. String or IDL/UUID changes made by this patch: None.

Looks like this was approved over IRC? https://hg.mozilla.org/releases/mozilla-aurora/rev/04c4de891e69

I think I crossed wires a bit with Lukas a bit when I was on my way out the door yesterday. Sorry, my bad. Anyway, it landed and stuck on both branches. But I can backout if Lukas prefers.

Comment on attachment 8403285 [details] [diff] [review] 0001-Bug-989083-Stop-leaking-aCallback-and-remove-aScope-.patch, v2 naw, it's ok - no harm done.