1038943 - Turn on leak checking for b2g mochitests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Which means adding support to the test harness.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

And now that I'm passing the correct environment in stuff breaks.  \o/

Comment 2

[Andrew Halberstadt [:ahal]]

Thanks, this looks good.. but I would put this in mozrunner instead of the mochitest harness. This would give us leak checking on reftest/marionette/xpcshell as well.

I don't mind taking this over if you don't mind waiting until next week sometime.

Comment 3

[(not currently active) Ted Mielczarek]

I actually had it on my TODO list to move the leak checking code to somewhere in mozbase, since we currently have it all in automationutils.py (and maybe duplicated in mochitest). mozrunner doesn't work because the xpcshell harness doesn't use it.

Comment 4

[Andrew Halberstadt [:ahal]]

Oh right, I think it's only B2G that uses mozrunner xpcshell, and I guess reftest doesn't use it yet at all.

Maybe a new 'mozleakcheck' module in mozbase?

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> I actually had it on my TODO list to move the leak checking code to
> somewhere in mozbase, since we currently have it all in automationutils.py
> (and maybe duplicated in mochitest). mozrunner doesn't work because the
> xpcshell harness doesn't use it.

Did you mean you were planning to do this soon, or at some vague point in the future? I can take a look too if you want. What do you think of a new module for leak checking?

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8456564 [details] [diff] [review]
Patch

I think ted and I agree that fixing this in mozbase is preferable to fixing it in mochitest. Unflagging.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8456564 [details] [diff] [review]
Patch

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

After talking to khuey on irc, it might be better to just land this for now and then move the leak processing over to mozbase along with everything else.

There's some horrible-ness around how mozrunner handles environment at the moment. Also around how runtestsb2g.py doesn't just instantiate a mozrunner object directly and instead relies on marionette to do it. This patch looks good, but I want to look into fixing a few things in mozrunner first so doing this isn't quite as difficult.

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Did you mean you were planning to do this soon, or at some vague point in
> the future? I can take a look too if you want. What do you think of a new
> module for leak checking?


Near-future when I do the reftest-on-mozbase work. Let's not block khuey's patch on that, it's pretty localized to Mochitest and provides clear value.

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: Do leak checking in b2g mochitests

This patch is basically the same as Kyle's except I fixed up DeviceRunner's handling of environment so it's possible to modify it after instantiation.

I'm getting some kind of build error, so haven't been able to test locally. In the meantime, here's a try run:
https://tbpl.mozilla.org/?tree=Try&rev=631bddbfd05b

Comment 10

[Andrew Halberstadt [:ahal]]

Fixes the dumb mistake:
https://tbpl.mozilla.org/?tree=Try&rev=5da543cadc70

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: Do leak checking in b2g mochitests

There was an extra comma in the last one: https://tbpl.mozilla.org/?tree=Try&rev=f400a2cb43d4

I'm not sure why debug mochitest-1 isn't being scheduled there.. I verified the proper environment is being passed in though.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Probably because debug has a different job name than opt. TryChooser special-cases them.

Comment 13

[Andrew Halberstadt [:ahal]]

Yeah, pushing only debug seems to work:
https://tbpl.mozilla.org/?tree=Try&rev=7c2f2256262c

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8461700 [details] [diff] [review]
Do leak checking in b2g mochitests

Debug tests definitely don't like that patch. Must be something in the environment.

Comment 15

[Andrew Halberstadt [:ahal]]

I noticed this in the logs:
INFO ### XPCOM_MEM_BLOAT_LOG defined -- unable to log bloat/leaks to /data/local/tests/tmp/runtests_leaks.log

Maybe because /data/local/tests/tmp doesn't exist? Here's a new push that fixes that, though I'm not optimistic that it will solve all the orange and red:
https://tbpl.mozilla.org/?tree=Try&rev=4cbb6fa82754

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

MOZ_DISABLE_NONLOCAL_CONNECTIONS is still set in the environment somehow.

Comment 17

[Andrew Halberstadt [:ahal]]

The latest try run dumps the env just before starting the b2g process:
08:06:53     INFO -    "MOZ_CRASHREPORTER": "1",
08:06:53     INFO -    "XPCOM_DEBUG_BREAK": "stack",
08:06:53     INFO -    "R_LOG_VERBOSE": "1",
08:06:53     INFO -    "NSPR_LOG_MODULES": "signaling:5,mtransport:5,datachannel:5",
08:06:53     INFO -    "MOZ_DISABLE_NONLOCAL_CONNECTIONS": null,
08:06:53     INFO -    "XRE_NO_WINDOWS_CRASH_DIALOG": "1",
08:06:53     INFO -    "GNOME_DISABLE_CRASH_DIALOG": "1",
08:06:53     INFO -    "R_LOG_DESTINATION": "stderr",
08:06:53     INFO -    "MOZ_CRASHREPORTER_NO_REPORT": "1",
08:06:53     INFO -    "NS_TRACE_MALLOC_DISABLE_STACKS": "1",
08:06:53     INFO -    "R_LOG_LEVEL": "6",
08:06:53     INFO -    "XPCOM_MEM_BLOAT_LOG": "/data/local/tests/log/runtests_leaks.log",
08:06:53     INFO -    "MOZ_GMP_PATH": "/builds/slave/test/build/xre/bin/gmp-fake"

Is it possible we need to set it to "0" instead of null? I don't understand how null could evaluate to true though:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsSocketTransport2.cpp#1179

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, we need to delete it entirely.  Try changing the 'update' bit to 'del'.

C's getenv returns a non-null pointer if the variable is set at all.

Comment 19

[Andrew Halberstadt [:ahal]]

Oh wait, this is just getting passed into 'sh', so it's probably being coerced to a string. Though that means that !!'0' would also be true. So we have to make sure not to set it at all. This might work:
https://tbpl.mozilla.org/?tree=Try&rev=a6a57775bdfd

Comment 20

[Andrew Halberstadt [:ahal]]

Heh, yeah, what khuey said ;)

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So that ran tests successfully but it doesn't look like it processed the leak log.  The runs should be bright orange with leaks.

Comment 22

[Andrew Halberstadt [:ahal]]

Attachment: Do leak checking in b2g mochitests

This one seems to work.

Comment 23

[Andrew Halberstadt [:ahal]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #21)
> So that ran tests successfully but it doesn't look like it processed the
> leak log.  The runs should be bright orange with leaks.

I see:

10:26:35     INFO -  2809 INFO Leaked URLs:
10:26:35     INFO -  2810 INFO   file:///system/b2g/omni.ja
10:26:35     INFO -  2811 INFO   chrome://b2g/content/shell.html
10:26:35     INFO -  2812 INFO   resource://gre-resources/ua.css
10:26:35     INFO -  2813 INFO   resource://gre-resources/html.css
10:26:35     INFO -  2814 INFO   chrome://global/content/minimal-xul.css
10:26:35     INFO -  2815 INFO   chrome://global/content/xul.css
10:26:35     INFO -  2816 INFO   resource://gre-resources/quirk.css
10:26:35     INFO -  2817 INFO   resource://gre-resources/full-screen-override.css
10:26:35     INFO -  2818 INFO   resource://gre/res/svg.css
10:26:35     INFO -  2819 INFO   resource://gre-resources/counterstyles.css
10:26:35     INFO -  2820 INFO   chrome://global/skin/scrollbars.css
10:26:35     INFO -  2821 INFO   resource://gre-resources/number-control.css
10:26:35     INFO -  2822 INFO   resource://gre-resources/forms.css
10:26:35     INFO -  2823 INFO   x:///chrome/chrome/content/shell.html
10:26:35     INFO -  2824 INFO   chrome://b2g/content/shell.css
10:26:35     INFO -  2825 INFO   chrome://b2g/content/shell.css
10:26:35     INFO -  2826 INFO   chrome://b2g/content/settings.js
10:26:35     INFO -  2827 INFO   chrome://b2g/content/shell.js
10:26:35     INFO -  2828 INFO   chrome://global/content/bindings/scrollbar.xml#scrollbar
10:26:35     INFO -  2829 INFO   chrome://global/content/bindings/scrollbar.xml
10:26:35     INFO -  2830 INFO   x:///chrome/toolkit/content/global/bindings/scrollbar.xml
10:26:35     INFO -  2831 INFO   chrome://global/content/bindings/scrollbar.xml
10:26:35     INFO -  2832 INFO   chrome://global/content/bindings/scrollbar.xml#thumb
10:26:35     INFO -  2833 INFO   chrome://global/content/bindings/scrollbar.xml
10:26:35     INFO -  2834 INFO   chrome://global/content/bindings/scrollbar.xml#scrollbar-base
10:26:35     INFO -  2835 INFO   chrome://global/content/bindings/scrollbar.xml#scrollbar
10:26:35     INFO -  2836 INFO   chrome://global/content/bindings/scrollbar.xml#scrollbar-base
10:26:35     INFO -  2837 INFO   chrome://global/content/bindings/scrollbar.xml#thumb
10:26:35     INFO -  2838 INFO   chrome://global/content/bindings/scrollbar.xml#scrollbar-base
10:26:35     INFO -  2839 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:35     INFO -  2840 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:36     INFO -  2841 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:36     INFO -  2842 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:36     INFO -  2843 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:36     INFO -  2844 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:36     INFO -  2845 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:36     INFO -  2846 INFO [Parent 669] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file ../../../gecko/xpcom/base/nsTraceRefcnt.cpp, line 145
10:26:36     INFO -  2847 INFO nsStringStats
10:26:36     INFO -  2848 INFO  => mAllocCount:         134469
10:26:36     INFO -  2849 INFO  => mReallocCount:         6055
10:26:36     INFO -  2850 INFO  => mFreeCount:          131544  --  LEAKED 2925 !!!
10:26:36     INFO -  2851 INFO  => mShareCount:         109945
10:26:36     INFO -  2852 INFO  => mAdoptCount:           5434
10:26:36     INFO -  2853 INFO  => mAdoptFreeCount:       5432  --  LEAKED 2 !!!
10:26:36     INFO -  2854 INFO  => Process ID: 669, Thread ID: 1074476168

I think maybe mozharness just needs to highlight the leaks. Which would be a separate patch. Also if it turns all the jobs bright orange, that means we can't land it right away.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The output of processLeakLog looks something like

TEST-UNEXPECTED-FAIL: ...

This leaked URL's stuff and the nsStringsStats stuff is there today (go grab a log off of mozilla-central and compare).

The harness has the ability to set a threshold value in bytes to not turn things orange, but I would like to prove that we can turn it orange first.

Comment 25

[Andrew Halberstadt [:ahal]]

Comment on attachment 8462698 [details] [diff] [review]
Do leak checking in b2g mochitests

Ok, will unflag again for now (sorry for the bugspam ted)

Comment 26

[Andrew Halberstadt [:ahal]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #24)
> The output of processLeakLog looks something like
> 
> TEST-UNEXPECTED-FAIL: ...
> 
> This leaked URL's stuff and the nsStringsStats stuff is there today (go grab
> a log off of mozilla-central and compare).
> 
> The harness has the ability to set a threshold value in bytes to not turn
> things orange, but I would like to prove that we can turn it orange first.

I'll have less time to look into this this week. Here's a try run with some extra debugging info, but barring something obvious coming out of there, I don't have any other ideas:
https://tbpl.mozilla.org/?tree=Try&rev=41db1caa6657

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The output not showing up was bug 1045255.

I pushed again.

https://tbpl.mozilla.org/?tree=Try&rev=daaaf70e17d0

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Based on https://tbpl.mozilla.org/?tree=Try&rev=4e53a54dd68b I set the threshold at 400000 bytes.

Comment 29

[Andrew Halberstadt [:ahal]]

Comment on attachment 8464116 [details] [diff] [review]
Patch

Flagging ted since I wrote part of this patch and he's planning on refactoring leak checking later.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8464116 [details] [diff] [review]
Patch

>-          "default": 0,
>+          "default": 400000,

What makes this specific to b2g?

(I think we used to have test-specific and platform-specific defaults somehow, back before we got all the leaks down to 0.  It might be worth seeing how we did that.)

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8464138 [details] [diff] [review]
Patch

>diff --git a/testing/mochitest/mochitest_options.py b/testing/mochitest/mochitest_options.py
>--- a/testing/mochitest/mochitest_options.py
>+++ b/testing/mochitest/mochitest_options.py
>@@ -750,16 +750,17 @@ class B2GOptions(MochitestOptions):
>         defaults = {}
>         defaults["httpPort"] = DEFAULT_PORTS['http']
>         defaults["sslPort"] = DEFAULT_PORTS['https']
>         defaults["logFile"] = "mochitest.log"
>         defaults["autorun"] = True
>         defaults["closeWhenDone"] = True
>         defaults["testPath"] = ""
>         defaults["extensionsToExclude"] = ["specialpowers"]
>+        defaults["leakThreshold"] = 400000
>         self.set_defaults(**defaults)

My understanding is that what you're asking me to review is the leak threshold.  This is fine with me, so r=dbaron.  At some point you're likely to want it to be a little more granular (per suite rather than for all suites), though.  (I sure hope B2GOptions is B2G-only, though somebody should probably double-check that.)

https://hg.mozilla.org/mozilla-central/rev/126af18be989 looks like the last time we had a nonzero leak threshold in mochitest, for what it's worth.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

With both of the dependent bugs fixed the threshold can probably be 5000 bytes instead of 400000.

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

review ping?

Comment 35

[(not currently active) Ted Mielczarek]

Comment on attachment 8464138 [details] [diff] [review]
Patch

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

::: testing/mochitest/mochitest_options.py
@@ +754,5 @@
>          defaults["autorun"] = True
>          defaults["closeWhenDone"] = True
>          defaults["testPath"] = ""
>          defaults["extensionsToExclude"] = ["specialpowers"]
> +        defaults["leakThreshold"] = 400000

Do you have a bug on file on reducing this to zero? Would be great to mention the bug number here.

::: testing/mozbase/mozrunner/mozrunner/base/device.py
@@ +32,4 @@
>      def __init__(self, device_class, device_args=None, **kwargs):
> +        process_log = tempfile.NamedTemporaryFile(suffix='pidlog')
> +        self.env['MOZ_PROCESS_LOG'] = process_log.name
> +        self.env.update(kwargs.pop('env', None) or {})

kwargs.pop('env', {})

@@ +32,5 @@
>      def __init__(self, device_class, device_args=None, **kwargs):
> +        process_log = tempfile.NamedTemporaryFile(suffix='pidlog')
> +        self.env['MOZ_PROCESS_LOG'] = process_log.name
> +        self.env.update(kwargs.pop('env', None) or {})
> +        self._env = self.env

Feels a little weird to mix the class variable with an instance variable. Maybe just do self._env = dict(self.env) and then modify self._env?

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I forgot to update the commit message :/

https://hg.mozilla.org/integration/b2g-inbound/rev/78613874d413

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And a bustage fix since Android is seriously fucked up.

https://hg.mozilla.org/integration/b2g-inbound/rev/d419d642fb4e

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And I went ahead and updated to the exact value so that any regression will be noticed.

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/b2g-inbound/rev/9a1da88e17e2

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/78613874d413
https://hg.mozilla.org/mozilla-central/rev/d419d642fb4e
https://hg.mozilla.org/mozilla-central/rev/9a1da88e17e2

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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