993580 - Various devtool test leaks when run in chunk-by-dir mode on Cedar

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

This occurs on all platforms with devtools running in their own test suite and chunk-by-dir mode enabled.

https://tbpl.mozilla.org/php/getParsedLog.php?id=37454733&tree=Cedar

10:34:56  WARNING -  TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_toolbar_webconsole_errors_count.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/devtools/framework/toolbox.xul]
10:34:56  WARNING -  TEST-UNEXPECTED-FAIL | browser/devtools/shared/test/browser_toolbar_webconsole_errors_count.js | leaked 1 window(s) until shutdown [url = about:blank]

Comment 1

[Ryan VanderMeulen [:RyanVM]]

2:49:08 PM - Optimizer: msucan, RyanVM|sheriffduty: clear cut, I see that we are not waiting for gDevTools.closeToolbox()
2:49:54 PM - Optimizer: that might be the cause of leaks. As previously, firefox.exe would be live for the toolbox to destroy properly (even after the test finish). But now, firefox.exe exits, without toolbox being destroyed completely.

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: patch

Can you try this on Cedar ?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Cedar&jobname=mochitest.*chrome&rev=f0fd885f5f7b

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: patr 2

followup for browser_editablemodel* test fix.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Cedar&jobname=mochitest.*chrome&rev=927a038208a2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

These two patches help, but we're still seeing some OSX leaks.

https://tbpl.mozilla.org/php/getParsedLog.php?id=37470108&tree=Cedar
https://tbpl.mozilla.org/php/getParsedLog.php?id=37470469&tree=Cedar

TEST-UNEXPECTED-FAIL | browser/devtools/layoutview/test/browser_editablemodel_allproperties.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/layoutview/view.xhtml]
TEST-UNEXPECTED-FAIL | browser/devtools/layoutview/test/browser_editablemodel_border.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/layoutview/view.xhtml]
TEST-UNEXPECTED-FAIL | browser/devtools/layoutview/test/browser_editablemodel_stylerules.js | leaked 2 window(s) until shutdown [url = chrome://browser/content/devtools/layoutview/view.xhtml]

Comment 7

[Ryan VanderMeulen [:RyanVM]]

CC Mossop and pbrosset as the authors/reviewers of the leaking layoutview tests from comment 6.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Also, we do sporadically hit them on Linux/Windows as well, but they're most frequent on OSX.

Comment 9

[Dave Townsend [:mossop]]

Strange...

Comment 10

[Patrick Brosset <:pbro>]

It doesn't leak for me locally, when running all layoutview tests (with a debug build). How does the chunk-by-dir mode works exactly?
Anyway, I think I'll give a shot at rewriting some parts of these layoutview tests and pushing to try, see what happens.

Comment 11

[Girish Sharma [:Optimizer]]

chunk by dir is equialent to running single directory tests locally like

mach mochitest-browser browser/devtools/webconsole/

So you are doing the right thing. leak not visible is a different issue :)

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

chunk-by-dir is just splitting the directories into a chunk instead of the test cases.  I am still working on run-by-dir which would be running a fresh instance of the profile/browser for  each subdir of tests.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Yeah, it's very important to note that with chunk-by-dir, the entire test run still reuses the same browser instance. All it means is that a directory won't get split across chunks and a new test won't change the split (unless you add an entire new directory).

More relevant here is that this didn't start happening until the push from comment 3. So maybe try applying that locally first?

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug993580-layoutview-test-leaks.patch

This is a quick rewrite of the tests. They're now more consistent with the styleinspector tests I just rewrote, and the destroy/close sequence has been redone, so the leak should be gone.

Comment 15

[Patrick Brosset <:pbro>]

RyanVM, can you try this on Cedar? See if that fixes the layoutview tests leaks?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8403549 [details] [diff] [review]
patr 2

https://tbpl.mozilla.org/?tree=Cedar&jobname=mochitest.*chrome&rev=6d03d1592b53

This also supersedes attachment 8403549 [details] [diff] [review], so marking it obsolete.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

And Try for good measure:
https://tbpl.mozilla.org/?tree=Try&rev=6050caaf4e30

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This still fails on both Cedar and Try.
https://tbpl.mozilla.org/php/getParsedLog.php?id=37513242&tree=Try

IRC discussion for posterity:
11:04:40 AM - pbrosset: RyanVM: yeah, so right now my feeling about this leak is that the layout-view panel (which is being tested) listens to mozafterpaint events, which it should not, and it uses it to refresh. So when the test ends, it's still trying to update itself, sending requests to the devtools server
11:04:45 AM - pbrosset: the leak probably comes from there
11:06:12 AM - pbrosset: RyanVM: bug 930436 is supposed to fix this repaint issue, but it's not going to land shortly
11:06:38 AM - pbrosset: RyanVM: having said this, this test wasn't leaking before
11:07:08 AM - RyanVM: pbrosset: yeah, the interesting thing is taht it wasn't a chunking change that made it start, it was the first patch from bug 993580 that did
11:09:04 AM - pbrosset: RyanVM: right, that's true. And now that you mention it, I see a whole lot of "a promise chain failed ..." errors in the logs
11:09:22 AM - pbrosset: none of them fail tests, but it definitely seems to be what causes the leak for the layoutview tests
11:09:27 AM - pbrosset: not 100% sure though

Comment 19

[Girish Sharma [:Optimizer]]

1:57:10 AM - Optimizer: i had a hunch that we leak layout view html pages but only if we close the toolbox
1:57:23 AM - Optimizer: thus my second patch revealed the leaks
1:57:41 AM - Optimizer: as it made the toolbox to be closed
1:57:47 AM - Optimizer: while previously it was never being closed
1:57:58 AM - Optimizer: (at the end of tests, before removing the tabs)
2:04:39 AM - Optimizer: RyanVM: want me to submit this hack fix ? or figure out the real cause ?
2:05:12 AM - RyanVM: Optimizer: Can you attach the hack fix with a small description for now and keep investigating?
2:05:38 AM - RyanVM: Optimizer: i suspect we want pbrosset's patch if at all possible
2:05:45 AM - RyanVM: but it's nice to have something that can work now otherwise
2:05:56 AM - Optimizer: his patch makes it impossible to not close toolbox for particular tests
2:06:41 AM - RyanVM: which is the real cause part?
2:07:57 AM - Optimizer: i didn't get u 
2:08:24 AM - RyanVM: i'm trying to understand what you mean by the "real cause" here
2:08:40 AM - RyanVM: and how that relates to pbrosset's patch making it impossible
2:09:43 AM - Optimizer: real cause is that something leaks in editable box model tests
2:09:55 AM - Optimizer: because at the time of toolbox close, something is not cleared properly
2:10:11 AM - Optimizer: but if you directly close the tab, without closing toolbox, nothing is there to leak
2:10:13 AM - Optimizer: i guess
2:10:21 AM - RyanVM: ok, so that was my question
2:10:23 AM - Optimizer: so before my patches, the toolbox was not closed
2:10:30 AM - Optimizer: ever. thus no leak.
2:10:34 AM - RyanVM: it sounds like the "real" fix is compatible, but the hack isn't
2:10:44 AM - Optimizer: pbrosset made is such that toolbox opening and closing is done from head.js
2:10:55 AM - Optimizer: compatible ?
2:11:34 AM - RyanVM: it's impossible to make it not leak in the head.js scenario?
2:12:59 AM - Optimizer: I dont know
2:13:06 AM - Optimizer: Mossop might
2:13:12 AM - Optimizer: "the real fix"
2:13:21 AM - RyanVM: ok, that sounds like what we need to answer then
2:13:39 AM - RyanVM: so if you don't mind running your patch through Try in the mean time and attaching it to the bug with your findings, that sounds like the way to go
2:14:13 AM - RyanVM: and it may end up meaning that we land your fix for now and let pbrosset or someone else sort out the real fix as part of the overall test refactoring
2:14:17 AM - RyanVM: and spin that off into another bug
2:16:25 AM - Optimizer: yeah
2:16:36 AM - Optimizer: I'll just paste this irc log

Comment 20

[Girish Sharma [:Optimizer]]

Attachment: leakfix

Combined final patch.

Lets keep this bug for unblocking Cedar and fix the real cause (along with test cleanup) in bug 994314

try push : https://tbpl.mozilla.org/?tree=Try&rev=a8f58dc39f6b

Ryan, can you push this to Cedar, backing out anything previously pushed ?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Cedar&jobname=mochitest.*chrome&rev=9abc18341efb

Comment 22

[Ryan VanderMeulen [:RyanVM]]

And a try run for good measure:
https://tbpl.mozilla.org/?tree=Try&rev=7eee9af25724

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8404231 [details] [diff] [review]
leakfix

Review of attachment 8404231 [details] [diff] [review]:
-----------------------------------------------------------------

Good call. Let's fix these quickly so we can move to cedar, and take care of the actual root cause and of the test rewrite in the other bug.
Thanks.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3277869ec180

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3277869ec180

Comment 26

[Girish Sharma [:Optimizer]]

Comment on attachment 8404231 [details] [diff] [review]
leakfix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 994747
User impact if declined: This bug fixes an issue introduced by 994747. Also, this patch is needed as a part of a bigger queue so as to uplift the chunks-by-dir feature of tests to aurora. see https://tbpl.mozilla.org/?tree=Try&rev=bfee331a418c
Testing completed (on m-c, etc.): mc
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 27

[Girish Sharma [:Optimizer]]

Previous form is wrong. this is correct form.

Comment on attachment 8404231 [details] [diff] [review] [diff] [review]
leakfix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): running tests by chunks-by-dir
User impact if declined: browser/devtools/shared/test/browser_toolbar_webconsole_errors_count.js will leak on aurora once chunks-by-dir is ported to aurora. Also, this patch is needed as a part of a bigger queue so as to uplift the chunks-by-dir feature of tests to aurora. see https://tbpl.mozilla.org/?tree=Try&rev=bfee331a418c
Testing completed (on m-c, etc.): mc
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b4ddc9970bc7