Closed Bug 1399854 Opened 2 years ago Closed 2 years ago

Intermittent browser/components/newtab/tests/browser/browser_newtab_overrides.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/downloads/contentAreaDownloadsView.xul]

Categories

(Firefox :: New Tab Page, defect, P5)

defect

Tracking

()

RESOLVED INCOMPLETE
Firefox 58
Tracking Status
firefox57 --- disabled
firefox58 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: ursula)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:race])

Attachments

(1 file)

this is failing quite often, sort of across all platforms, but on debug- it appears to be specific to browser_newtab_overrides.js.

:tspurway, can you help prioritize this and find someone to look at it?
Flags: needinfo?(tspurway)
Whiteboard: [stockwell needswork]
:mardak, any thoughts on this one?
Flags: needinfo?(tspurway) → needinfo?(edilee)
We did touch `aboutNewTabService` in bug 1398819.

https://hg.mozilla.org/mozilla-central/rev/8475a8df9f16

It made it to m-c on the 13th, and the test failures did start on the 14th.

Not sure how it's causing leaks, but ursula did notice some odd leaking in bug 1396274 that resulted in just disabling aboutHome tests. However, this particular leaking test, we probably do want to keep testing the override behavior.
Flags: needinfo?(edilee) → needinfo?(khudson)
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc34fa77f39
Skip test browser_newtab_overrides.js for 200+ failures; r=me,test-only
Keywords: leave-open
Whiteboard: [stockwell disable-recommended] → [stockwell disabled]
Assignee: nobody → usarracini
I'm taking a look at this
Flags: needinfo?(khudson)
ursula, great investigating in https://github.com/mozilla/activity-stream/issues/3578#issuecomment-340877660

mikedeboer, looks like this intermittent is caused by browser_newtab_overrides.js mysteriously using chrome://…contentAreaDownloadsView.xul as a about:newtab override (i.e., loading about:newtab internally loads the chrome: url)

ursula commented out `//this._downloadsData.addView(this);` from allDownloadsViewOverlay.js in a try run (among other debug guesses), and the leak seems to go away. My guess is that the window gets added to Downloads.jsm global module's `.getList` and doesn't get cleaned up in time (there is a window "unload" listener that `this._downloadsData.removeView(this);` and this leak is intermittent).

Looking through the history of the test doesn't seem to show good reason to be even using contentAreaDownloadsView.xul in the first place, so our fix is probably to just point to some other page.. about:blank should be safe…

Do you have any ideas other suggestions for this intermittent leak?
Flags: needinfo?(mdeboer)
Comment on attachment 8923970 [details]
Bug 1399854 - Intermittent browser/components/newtab/tests/browser/browser_newtab_overrides.js leaked 2 windows until shutdown

https://reviewboard.mozilla.org/r/195142/#review200180

Didn't realize activity-stream.enabled was false for the test. We probably should start thinking about how things should behave when the pref goes away… but not for this bug! ;)

::: browser/components/newtab/tests/browser/browser.ini:3
(Diff revision 1)
>  [DEFAULT]
> -support-files =
> -  dummy_page.html
> +prefs =
> +  browser.newtabpage.activity-stream.enabled=false

Let's keep this in the (now only one) test. We'll probably be adding tests that use the default (true) especially going forwards when the pref won't exist and effectively be true.
Attachment #8923970 - Flags: review?(edilee) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d6373b256155 -d 305151029850: rebasing 431339:d6373b256155 "Bug 1399854 - Intermittent browser/components/newtab/tests/browser/browser_newtab_overrides.js leaked 2 windows until shutdown r=Mardak" (tip)
local [dest] changed browser/components/newtab/tests/browser/browser_remotenewtab_pageloads.js which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging browser/components/newtab/tests/browser/browser_newtab_overrides.js
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4c0f4165f13
Intermittent browser/components/newtab/tests/browser/browser_newtab_overrides.js leaked 2 windows until shutdown r=Mardak
Well, my guess would be that this page is particularly unfit to be used in a test like this, since it contains a <window> element and its not used as such.

Apparently it's only used in Private Browsing mode, so I'm curious whether we should look forward to _not_ abusing this XUL-based interface and start working on a proper HTML version, instead.
But Paolo's a much better person to propose that to ;-) What do you think, Paolo, or is moving about:downloads to HTML already on a roadmap somewhere? Something to think about, perhaps, that may elevate the priority for such a rewrite: is the 'unload' fired consistently when an about:downloads tab is closed on the in-content window element or will the JS objects be kept alive until the main browser window unloads?
Flags: needinfo?(mdeboer) → needinfo?(paolo.mozmail)
(In reply to Mike de Boer [:mikedeboer] from comment #24)
> Paolo, or is moving about:downloads to HTML already on a roadmap somewhere?

I don't think this will become HTML before the browser UI does, as we want to reuse as much of the code as possible.

> Something to think about, perhaps, that may elevate the priority for such a
> rewrite: is the 'unload' fired consistently when an about:downloads tab is
> closed on the in-content window element or will the JS objects be kept alive
> until the main browser window unloads?

During normal execution, I believe that the unload event will always fire eventually, and objects scoped to the window will be freed when the window closes. Everything else created by the JSM modules will be kept for the duration of the session. While testing, leaks may definitely occur if we don't wait for the unload to complete. Converting to HTML wouldn't change this, anyways.
Flags: needinfo?(paolo.mozmail)
Keywords: leave-open
Whiteboard: [stockwell disabled] → [stockwell fixed:race]
Target Milestone: --- → Firefox 58
https://wiki.mozilla.org/Bugmasters#Intermittent_Test_Failure_Cleanup
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.