1392648 - SHIELD browser-chrome tests are going to permaleak when Gecko 57 merges to Beta on 2017-09-20

Status: VERIFIED FIXED

(Firefox :: Normandy Client, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Permaleaks on the next merge day

This looks disturbingly like bug 1391117, except now both OSX and Windows are affected :(

https://treeherder.mozilla.org/logviewer.html#?job_id=124764273&repo=try

TEST-UNEXPECTED-FAIL | leakcheck | default process: 8584 bytes leaked (BackstagePass, CondVar, IdlePeriod, Mutex, SharedScriptableHelperForJSIID, ...)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Also affects Linux on slower platforms (like the CCov runs).

Comment 2

[Osmose [:osmose, :mkelly]]

You know, given that your bisect pointed to the sessionstore changes felipe made in the other bug, I bet this is related to the fact that our tests emit "sessionstore-windows-restored" to test how we respond to the event. We should probably do something instead of that so that we don't trigger any other observers for that event.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Ah good catch! that must be it. There's an alternate notification for this purpose! See http://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/browser/components/sessionstore/SessionStore.jsm#4472

Comment 4

[Osmose [:osmose, :mkelly]]

RyanVM: I have a patch that I think will stop our tests from triggering the leak (although it doesn't fix the root cause, I still think it's a worthwhile change), but I'm not sure how I'd do a try run to test this, as I can't find other pushes beyond the one linked above that are hitting the failure, and I can't replicate the leak locally.

Would an OSX debug build running the mochitests hit it reliably?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/try/rev/cd680b6f5f1f3e985494b552f473cbccc6eed135 should suffice for reproducing. It won't reproduce from regular trunk (this is specific bustage that occurs on 57 in a Beta build configuration). Otherwise, if you want to attach the patch here, I can include it in my next run. I've got a patch stack to work around some other known issues so the results are a bit easier to go through.

Comment 6

[Osmose [:osmose, :mkelly]]

Attachment: Bug 1392648: Avoid using sessionrestore event in RecipeRunner tests.

Review commit: https://reviewboard.mozilla.org/r/173338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173338/

Comment 7

[Osmose [:osmose, :mkelly]]

Attached the patch, if it works out well on your next test run then I'll add a reviewer. Thanks!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Works great for the SHIELD test directory.

Comment 9

[Osmose [:osmose, :mkelly]]

Gijs is out until Friday, and felipe's pretty familiar with this failure. :P

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8901906 [details]
Bug 1392648: Avoid using sessionrestore event in RecipeRunner tests.

https://reviewboard.mozilla.org/r/173338/#review179224

::: browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm:107
(Diff revision 1)
> +    switch (topic) {
> +      case PREF_CHANGED_TOPIC:
> +        this.observePrefChange(data);
> +        break;
> +      case UI_AVAILABLE_TOPIC:
> +        this.observeUIAvailable();

it's possible that this might show up in the Browser Console as "promise with unhandled exception". You might want to at a minimum add a `.catch(()=>{})` here

Comment 11

[Osmose [:osmose, :mkelly]]

Comment on attachment 8901906 [details]
Bug 1392648: Avoid using sessionrestore event in RecipeRunner tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/173338/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac60c5f7d3a4
Avoid using sessionrestore event in RecipeRunner tests. r=Felipe

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ac60c5f7d3a4