1317867 - Intermittent devtools/client/netmonitor/test/browser_net_footer-summary.js | leaked 2 window(s) until shutdown [url = about:devtools-toolbox]

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=6645729&repo=autoland

https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-win32-debug/1479196153/autoland_win7_vm-debug_test-mochitest-devtools-chrome-1-bm139-tests1-windows-build535.txt.gz

Comment 1

[Intermittent Failures Robot]

22 failures in 119 pushes (0.185 failures/push) were associated with this bug yesterday.  

Repository breakdown:
* autoland: 14
* mozilla-inbound: 8

Platform breakdown:
* windows7-32-vm: 12
* linux32: 9
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1317867&startday=2016-11-16&endday=2016-11-16&tree=all

Comment 2

[Intermittent Failures Robot]

33 failures in 147 pushes (0.224 failures/push) were associated with this bug yesterday.  

Repository breakdown:
* autoland: 14
* mozilla-inbound: 10
* try: 6
* mozilla-central: 3

Platform breakdown:
* windows7-32-vm: 18
* linux32: 14
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1317867&startday=2016-11-17&endday=2016-11-17&tree=all

Comment 3

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1317867 - Fixed window leak in browser_net_footer-summary.js mochitest

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

Comment 4

[Jarda Snajdr [:jsnajdr]]

How the leak happens:

1. The test require()'s the selectors module and uses it. It doesn't use the module already loaded in the Netmonitor window, but loads its own instance in its own loader.

2. The selectors module contains memoized selectors, i.e., the last returned value is remembered inside the selector.

3. The memoized value keeps a reference to the Netmonitor window. More precisely, the value object contains a function, that function has a closure, and that closure (aka lexical environment) contains the window global.

4. After the test is finished, it checks for window and docshell leaks. At that time, the test tab is closed, the toolbox is closed, too, so all its DOM windows should be gone.

5. But the selectors module is still present in the test loader, so the references to DOM windows still exist - leak detected!

How to solve this?

Keep a strict distinction between the application code we are testing, and the test code that does the testing. Don't load application modules into the test loader, but use the application loader instead.

I'm attaching a patch that fixed the only leak that's happening so far, but we should fix several other tests that aren't leaking yet:

- many tests are using the L10N module. Don't load a new instance into the test, but use the already existing instance owned by the application.
- as more and more parts are converted to Redux, the tests use the modules for actions and selectors, so that they can trigger actions and inspect the application state. All these modules should be accessed using the application loader. No separate instances in the test loader.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8812120 [details]
Bug 1317867 - Fixed window leak in browser_net_footer-summary.js mochitest

https://reviewboard.mozilla.org/r/94006/#review94162

Very nice!

Thanks for detailed explanation, you could also put
a comment directly into the test file.

Honza

Comment 6

[Jarda Snajdr [:jsnajdr]]

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> Thanks for detailed explanation, you could also put
> a comment directly into the test file.

Eventually, every mochitest should use winRequire like this one. Should we explain it everywhere? It's not a one-off exception.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d0345ea4344
Fixed window leak in browser_net_footer-summary.js mochitest r=Honza

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jarda Snajdr [:jsnajdr] from comment #6)
> (In reply to Jan Honza Odvarko [:Honza] from comment #5)
> > Thanks for detailed explanation, you could also put
> > a comment directly into the test file.
> 
> Eventually, every mochitest should use winRequire like this one. Should we
> explain it everywhere? It's not a one-off exception.
Yeah I know. It doesn't make much sense to repeat the same comment in every test, but
it would be good to have it somewhere. Perhaps a new mochitest-tips.md file?

Honza


Honza

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d0345ea4344

Comment 10

[areya]

https://hg.mozilla.org/mozilla-central/rev/7d0345ea4344

Comment 11

[Intermittent Failures Robot]

77 failures in 715 pushes (0.108 failures/push) were associated with this bug in the last 7 days. 

This is the #31 most frequent failure this week. 

** This failure happened more than 50 times this week! Resolving this bug is a high priority. **

Repository breakdown:
* autoland: 34
* mozilla-inbound: 29
* mozilla-central: 8
* try: 6

Platform breakdown:
* windows7-32-vm: 45
* linux32: 30
* windows8-64: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1317867&startday=2016-11-14&endday=2016-11-20&tree=all