Closed Bug 1325136 Opened 3 years ago Closed 3 years ago

TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_advanced_siteData.js | leaked 2 window(s) until shutdown [url = about:preferences#advanced]

Categories

(Firefox :: Preferences, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: jmaher, Assigned: Fischer)

References

Details

Attachments

(3 files)

this is a perma leak since the test was introduced, BUT- we don't see this as the leak by itself doesn't turn a job orange with a recent change to use structured logs in mochitest.

This is seen on win7, win8, and linux64, and osx on  e10s debug runs.

I assume this was not seen on try as the jobs were green, and of course for the last 5 days they have been green :)
:Fischer, can you take a look at this to see if we can fix the leaks?
Flags: needinfo?(fliu)
(In reply to Joel Maher ( :jmaher) from comment #0)
> this is a perma leak since the test was introduced, BUT- we don't see this
> as the leak by itself doesn't turn a job orange with a recent change to use
> structured logs in mochitest.

When fixing this (presumably in bug 1325148?) can we please make sure we include a regression test so that we don't break this again? So, a test that deliberately leaks where we verify that that is accurately detected?
Flags: needinfo?(ahalberstadt)
I won't be able to as part of the fix as we don't have a harness capable of this at the moment and getting one stood up is probably not something we want to block the fix on. That said, when we do stand up some kind of test harness harness I'll make sure leaks, assertions and crashes are the first things we add tests for.

It's pretty bad that we don't have test harness tests yet, and I think it's a failing most of us on the ateam are aware of and feel bad about.. I'll see if I can get it prioritized for Q1 and failing that, I have an idea for a hacky solution I think I could finish up in my spare time (bug 1311991).
Flags: needinfo?(ahalberstadt)
Have we verified that backing out the changes from bug 1312361 fixes the leaks? If so, unless there's something really obvious wrong with what the test does, I think we should back out the changes from bug 1312361 minus the added strings, and reland when this is fixed. I don't want to risk releasing something that leaks the world (which tends to be the consequence of leaking anything in browser tests, if it's not an oversight in the test itself).
Blocks: 1312361
Flags: needinfo?(jmaher)
I am awaiting test results, but then I realized- the backout would be removing the leaking test case, I will still see what else remains.
Flags: needinfo?(jmaher)
there are no other leaks in the same directory after a backout.
Assignee: nobody → fliu
Flags: needinfo?(fliu)
Found the fix: Should remove the registered observer on window unload.
Run the test locally with the fix, then no leak appears.
See [1] and [2] (No matter leak or no_leak, both the test results show PASS. Have to search "TEST-UNEXPECTED-FAIL" to see if leak failure was there).

Now is testing on Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a673d01e21ba76e5f4e5f89c6e9f0b9ebb9866e8

[1] attachment 8821060 [details]: 1325136_mochitest_local_leak.log
[2] attachment 8821061 [details]: 1325136_mochitest_local_no_leak.log
Hi Joel,

I scanned the test log of e10 bc4 on Linux x64 debug and the log of e10s bc1 on OS X 10.10 debug.
There is no leak failure caused by the browser_advanced_siteData.js test .
However, there are too many tests.
Do you know any good way to check if the failure is really fixed ?

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a673d01e21ba76e5f4e5f89c6e9f0b9ebb9866e8

Thanks.
Flags: needinfo?(jmaher)
I looked at the log file and searched for the test name, you can see it ran and was test-ok.  looking in a failing log if you open up bc4 (or whichever chunk it is in) you can search for the test name and you will see much more data, finally ending up with an error about the leak.

Thanks for fixing this so quickly!
Flags: needinfo?(jmaher)
Attachment #8821057 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8821057 [details]
Bug 1325136 - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_advanced_siteData.js | leaked 2 window(s) until shutdown [url = about:preferences#advanced],

https://reviewboard.mozilla.org/r/100444/#review101018

::: browser/components/preferences/in-content/advanced.js:61
(Diff revision 1)
>      this.updateActualAppCacheSize();
>  
>      if (Services.prefs.getBoolPref("browser.storageManager.enabled")) {
>        Services.obs.addObserver(this, "sitedatamanager:sites-updated", false);
> +      window.addEventListener("unload", () => {
> +        Services.obs.removeObserver(this, "sitedatamanager:sites-updated", false);

removeObserver takes only 2 parameters.
Attachment #8821057 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #13)
> > +      window.addEventListener("unload", () => {
> > +        Services.obs.removeObserver(this, "sitedatamanager:sites-updated", false);
> 
> removeObserver takes only 2 parameters.

Thanks, updated.
Test manifest should also be fixed to un-whitelist this test.
Flags: needinfo?(fliu)
right now we don't have a whitelist landed, in the final patch up for review, we have this test disabled via skip-if in browser.ini.  When this lands and I prove it works on try, I will make sure it is cleaned up- but we should all make sure that is the case :)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9947679164c8
TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_advanced_siteData.js | leaked 2 window(s) until shutdown [url = about:preferences#advanced], r=Gijs
Keywords: checkin-needed
(In reply to Joel Maher ( :jmaher) from comment #19)
> right now we don't have a whitelist landed, in the final patch up for
> review, we have this test disabled via skip-if in browser.ini.  When this
> lands and I prove it works on try, I will make sure it is cleaned up- but we
> should all make sure that is the case :)
Hi Joel,
What is the "whitelist" you and Masatoshi mentioned? Is there any required modification being missed?
Thank you.
Flags: needinfo?(fliu) → needinfo?(jmaher)
this is all currently in a patch to be landed in bug 1325148.  I am managing it until that bug is resolved and will make sure we follow up on it after it is resolved.
Flags: needinfo?(jmaher)
Hi Wes,

This patch is ready to land and the bug 1325148 depends on this bug.
Would you please help it out?

Thanks
Flags: needinfo?(wkocher)
https://hg.mozilla.org/mozilla-central/rev/9947679164c8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
This has landed already (comment 20).
Flags: needinfo?(wkocher)
You need to log in before you can comment on or make changes to this bug.