989083 - Fix and re-enable browser_tabview_bug643392.js,browser_tabview_bug628061.js,browser_tabview_bug650280_perwindowpb.js

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0df0f2fe3bbf

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 9

[Phil Ringnalda (:philor)]

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.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-989083-Clear-callback-and-scope-variables-in-asy.patch

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.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=84c80344ddcc

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/916460a17836

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Try looks good afaict.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-989083-Stop-leaking-aCallback-and-remove-aScope-.patch, v2

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 20

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=45d0e2cc146e

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

(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 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/7292ef816dc8

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7292ef816dc8

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Looks like this was approved over IRC?

https://hg.mozilla.org/releases/mozilla-aurora/rev/04c4de891e69

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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 28

[Lukas Blakk [:lsblakk] use ?needinfo]

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.