The default bug view has changed. See this FAQ.

Mozleak should use the error level instead of the warning level

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

({regression})

51 Branch
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [leave-open])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 4 obsolete attachments)

Recently mochitest stopped turning jobs orange with leaks. This was regressed by using the StructuredOutputParser in mozharness because mozleak was previously depending on the string "TEST-UNEXPECTED-FAIL" to turn the job orange.

Mozleak should instead log leak errors at the "error" level. Note, it will need to continue to contain the string "TEST-UNEXPECTED-FAIL" for test suites that don't use structured logging.

Updated

3 months ago
Depends on: 1325149

Updated

3 months ago
Depends on: 1325158
Comment hidden (mozreview-request)
This patch seems to fix the issue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46239c8ae40ed066d6ed784a1d91a0811c71f7df

Reftest was also affected. We'll need to either fix or backout all the leaks that show up in that try push before landing this.

Comment 3

3 months ago
mozreview-review
Comment on attachment 8820841 [details]
Bug 1325148 - Use "error" level in mozleak when logging leak failures,

https://reviewboard.mozilla.org/r/100242/#review100788

Subject to finding a workaround for the actual issues ofc.
Attachment #8820841 - Flags: review?(james) → review+
Depends on: 1325215
Depends on: 1325274
Depends on: 1325275
Depends on: 1325277
Whitelisted bugs should have highest Importance because they would have caused backout if mozleak had worked.
Agreed, once I have this landed (hopefully today or tomorrow), I'll send out a dev.platform post to solicit help and explain what happened. Each one of these will need to be looked into.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Here is the latest try push (the f8 error has been fixed in the above commits already):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a0da8521414bfcd53df7f7c9abb88c8995771a2

Joel, I'm going to be driving all day and then PTO until Wednesday. If the try run looks green and you give these an r+, please land them for me! If the try run is not green, you may need to add another skip rule to the second commit. There were some intermittent leaks I noticed that I purposefully did not whitelist. I figured as long as their frequency wasn't *too* high, it would be better to treat them as normal intermittents from here on out, to make it easier to find developers to fix them. If after landing, these intermittent leaks are too frequent, we can always add them to the whitelist after the fact.

I'll still check in during my PTO to help make sure this gets landed soon. Sorry for dropping it in your lap (worst Christmas present ever ;)). Hopefully, all that is required will be clicking the autoland button.

Comment 10

3 months ago
mozreview-review
Comment on attachment 8821156 [details]
Bug 1325148 - Temporarily disable mochitest leakchecking for directories containing known leaks,

https://reviewboard.mozilla.org/r/100524/#review101050

::: testing/mochitest/runtests.py:2219
(Diff revision 1)
> +            # they get fixed.
> +
> +            info = mozinfo.info
> +            skip_leak_conditions = [
> +                (options.flavor == 'browser' and d == 'toolkit/modules/tests/browser' and info['os'] == 'linux', 'bug 1325149'),  # noqa
> +                (options.flavor == 'browser' and d == 'toolkit/modules/tests/browser' and info['os'] == 'linux', 'bug 1325136'),  # noqa

this has the wrong directory, it should be:
browser/components/preferences/in-content/tests/

but the test browser_advanced_siteData.js has a patch that fixes the problem, so I think we could ignore this
Attachment #8821156 - Flags: review?(jmaher) → review+

Comment 11

3 months ago
mozreview-review
Comment on attachment 8821157 [details]
Bug 1325148 - Temporarily disable leakchecking in crashtests on linux,

https://reviewboard.mozilla.org/r/100526/#review101052
Attachment #8821157 - Flags: review?(jmaher) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

3 months ago
mozreview-review-reply
Comment on attachment 8821156 [details]
Bug 1325148 - Temporarily disable mochitest leakchecking for directories containing known leaks,

https://reviewboard.mozilla.org/r/100524/#review101050

> this has the wrong directory, it should be:
> browser/components/preferences/in-content/tests/
> 
> but the test browser_advanced_siteData.js has a patch that fixes the problem, so I think we could ignore this

Good catch, fixed. It's probably safer to leave it in for now so we don't have to block on the other fix landing and being merged around.

Also noticed there's a bunch of leaks in the try run still :(.. Must be intermittents, re-triggered to find out. It might also be that they aren't directory specific, in which case I guess we need to disable checking on the whole flavor/platform?
I see mochitest-plain: tests/dom/xhr/tests

that might be it- when more data comes in, we can queue up final patches and deploy.

Updated

3 months ago
Depends on: 1325438
adding just tests/dom/xhr/tests to the list and verifying this is all good on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6b8778af8354d5b20a4e1eecd658bd1350aa077
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
there are a series of leaks which occur frequently enough but what looks to be across various directories, both in mochitest-plain and in browser-chrome.  This is not something we can easily whitelist :(

One thing is we have a leak from a test case (not a bloat log) which doesn't work with this method- we need to manifest it off.
Depends on: 1325678
and more leaks I am hacking on, maybe a couple more try pushes and this will be good- lets see if I can get it by the morning.
Created attachment 8821731 [details] [diff] [review]
final patch here, I had to fix a flake8 error on your original patch
Attachment #8820841 - Attachment is obsolete: true
Attachment #8821731 - Flags: review?(ahalberstadt)
Created attachment 8821732 [details] [diff] [review]
final patch here, a few tests are disabled via manifest
Attachment #8821156 - Attachment is obsolete: true
Attachment #8821732 - Flags: review?(ahalberstadt)
no changes in the 3rd patch, here is a try push with all the goodness:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df8e1be85a71020858866a1fb61d907e0822e72

I believe we have at least one of the leaks fixed, I am waiting for it to get merged to m-c before editing any patches- in the meantime we need to get this landed asap.
Created attachment 8821872 [details] [diff] [review]
hack mochitests with whitelist and manifest hacks
Attachment #8821732 - Attachment is obsolete: true
Attachment #8821732 - Flags: review?(ahalberstadt)
Attachment #8821872 - Flags: review?(ahalberstadt)
Comment on attachment 8821731 [details] [diff] [review]
final patch here, I had to fix a flake8 error on your original patch

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

Thanks
Attachment #8821731 - Flags: review?(ahalberstadt) → review+
Attachment #8821872 - Flags: review?(ahalberstadt) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I took your latest patches and pushed them into the review request. You probably had a try run going, but here's another one based on the latest central:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26a6395e75a2113d4551acaaa7c311f659a35444

I'll check in again tonight to see if anything else needs to be whitelisted/disabled.
Attachment #8821731 - Attachment is obsolete: true
Attachment #8821872 - Attachment is obsolete: true

Comment 36

3 months ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee7a21566602
Use "error" level in mozleak when logging leak failures, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/193d05c4b3fc
Temporarily disable leakchecking in crashtests on linux, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/95ad94f3ef4b
Temporarily disable mochitest leakchecking for directories containing known leaks, r=jmaher
Created attachment 8822024 [details] [diff] [review]
enable the advanced_site.js test as the fix landed
Attachment #8822024 - Flags: review?(ahalberstadt)
Attachment #8822024 - Flags: review?(ahalberstadt) → review+
Note to sheriffs:
The three patches that landed get leak checking working again, so I expect that you'll see a rise in intermittents. So please don't back this out for "causing" intermittents, as it's really just making sure they're visible. If there are leak related intermittents that are too frequent, we can add that leak to the whitelist.

Joel and I will work to find people to help fix all the bustage that slipped through ASAP. I'll also do a newsgroup post explaining what happened and how we can prevent it from happening agian. Setting [leave-open] so Joel can land his patch too.
Whiteboard: [leave-open]
I backed it out for Linux x64 asan leaks (c3 seems to be permafail, dt5 intermittent):

https://hg.mozilla.org/integration/autoland/rev/afa60933f33519b9a901213338a69ab383165790
https://hg.mozilla.org/integration/autoland/rev/cb81231a73fbae2ae5d110a534e940e65697c394
https://hg.mozilla.org/integration/autoland/rev/9278da08b49c7788784c2b8383629ed3a685bd12

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=95ad94f3ef4b3df6453f2d295802951c208b1814
Failure log: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=95ad94f3ef4b3df6453f2d295802951c208b1814

[task 2016-12-27T11:33:29.598462Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at AllocateProtoAndIfaceCache, xpc::CreateGlobalObject, XPCWrappedNative::WrapNewGlobal, nsXPConnect::InitClassesWithNewWrappedGlobal
[task 2016-12-27T11:33:29.598561Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, NewEmptyScopeData, js::FunctionScope::copyData
[task 2016-12-27T11:33:29.598917Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::Shape::hashify, maybeCreateTableForLookup
[task 2016-12-27T11:33:29.599000Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, AllocScriptData, js::detail::CopyScript
[task 2016-12-27T11:33:29.599365Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_new, HashChildren, js::PropertyTree::insertChild, js::PropertyTree::getChild
[task 2016-12-27T11:33:29.599738Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::NewStringCopyNDontDeflate, AtomizeAndCopyChars
[task 2016-12-27T11:33:29.599827Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::NewStringCopyNDontDeflate, js::JSONParser
[task 2016-12-27T11:33:29.599904Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, js::Scope::clone
[task 2016-12-27T11:33:29.600300Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, _ZL17NewStringDeflatedILN2js7AllowGCE0EEP12JSFlatStringPNS0_16ExclusiveContextEPKDsm, AtomizeAndCopyChars
[task 2016-12-27T11:33:29.600696Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_new, js::ProxyObject::objectMovedDuringMinorGC, js::TenuringTracer::moveObjectToTenured, js::TenuringTracer::moveToTenured
[task 2016-12-27T11:33:29.600759Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at unknown stack
[task 2016-12-27T11:33:29.601142Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, _ZL17NewStringDeflatedILN2js7AllowGCE1EEP12JSFlatStringPNS0_16ExclusiveContextEPKDsm, JS_NewUCStringCopyN
[task 2016-12-27T11:33:29.601230Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::Shape::hashify, js::NativeObject::toDictionaryMode
[task 2016-12-27T11:33:29.601604Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, NewEmptyScopeData, js::GlobalScope::copyData
[task 2016-12-27T11:33:29.601967Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, js::FunctionScope::clone
[task 2016-12-27T11:33:29.602065Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, tryNewTenuredObject, js::Allocate
[task 2016-12-27T11:33:29.602461Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::NewStringCopyNDontDeflate, NewStringCopyN
[task 2016-12-27T11:33:29.602555Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::GlobalObject::getOrCreateDebuggers, js::Debugger::addDebuggeeGlobal
[task 2016-12-27T11:33:29.602646Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at nsStringBuffer::Alloc, nsAttrValue::GetStringBuffer, nsAttrValue::SetTo, mozilla::dom::Element::SetAttr
[task 2016-12-27T11:33:29.602731Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, pod_malloc, extractOrCopyRawBuffer
[task 2016-12-27T11:33:29.603366Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, AllocScriptData, partiallyInit
[task 2016-12-27T11:33:29.603450Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_calloc, maybe_pod_calloc, AllocScriptData, JSScript::partiallyInit
[task 2016-12-27T11:33:29.603831Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, js::GlobalScope::copyData
[task 2016-12-27T11:33:29.604208Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, CopyScopeData, CopyScopeData
[task 2016-12-27T11:33:29.604296Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, AllocChars, JSRope::flattenInternal
[task 2016-12-27T11:33:29.604379Z] 11:33:29    ERROR - TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at js_pod_malloc, maybe_pod_malloc, js::LazyScript::CreateRaw, js::LazyScript::Create
Flags: needinfo?(ahalberstadt)
Please read comment #39.
Depends on: 1325947
The c3 error looks like it's happening in toolkit/components/extensions/test/mochitest, which looks like bug 1325158. The reason it wasn't already being skipped is because it's a shutdown leak, so making shutdown leaks respect the "disable_leak_checking" flag in my patch should cover it.
Flags: needinfo?(ahalberstadt)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
I also documented the c3 leak here a bit more in bug 1325947
Comment hidden (mozreview-request)
Comment on attachment 8822024 [details] [diff] [review]
enable the advanced_site.js test as the fix landed

I folded this change into the main series. The fix it depends on has landed and been merged around.

Also here is a try run that I think should fix the ASAN c3 leak:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf372384e55c4441b0c6ac388c7e20b2607bbf19
Attachment #8822024 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 49

3 months ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3afc02948984
Use "error" level in mozleak when logging leak failures, r=jgraham
https://hg.mozilla.org/integration/autoland/rev/90dd139d7578
Temporarily disable leakchecking in crashtests on linux, r=jmaher
https://hg.mozilla.org/integration/autoland/rev/cf567de3d614
Temporarily disable mochitest leakchecking for directories containing known leaks, r=jmaher
Try run that actually fixed the c3 leak is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=792c4926ad8397e3c87f24e4f5cb2e70aa252f40

Please let Joel or I know if there is further bustage so we can hopefully fix it live rather than backing it out. This is one of those cases where the longer it remains unlanded the more potential bustage will slip through.

Comment 51

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3afc02948984
https://hg.mozilla.org/mozilla-central/rev/90dd139d7578
https://hg.mozilla.org/mozilla-central/rev/cf567de3d614
Per Joel, this probably affects 52 as well.
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
Keywords: regression
Version: unspecified → 52 Branch
Blocks: 1261194, 1261199
[Tracking Requested - why for this release]:  Shipping leaks is bad

Does this not affect 51?  At least bug 1261199 goes back to 51; not sure about other uses of the structured log parser.  So why is this not an issue on 51?

Can we get backports of this to the branches that are affected, please, and set the tracking flags on the blocking bugs accordingly, so we don't ship those leaks?
status-firefox51: unaffected → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Flags: needinfo?(ahalberstadt)
The patches in this bug disabled the browser_Heartbeat.js test, right?  Why?
bz, that was leaking on all platforms, we could narrow that down to something specific like debug or asan.  It is a brand new test that landed just last week.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #53)
> [Tracking Requested - why for this release]:  Shipping leaks is bad
> 
> Does this not affect 51?  At least bug 1261199 goes back to 51; not sure
> about other uses of the structured log parser.  So why is this not an issue
> on 51?
> 
> Can we get backports of this to the branches that are affected, please, and
> set the tracking flags on the blocking bugs accordingly, so we don't ship
> those leaks?

Only reftest was at risk for firefox 51, mochitest is 52 and later. But I'll backport this patch to both 52 and 51. It has been a bit of a scramble just to get the fix landed on central up until this point.

As for the individual leaks, we have been checking for them on 52 (and 51 for that one reftest leak). I haven't had a chance to go through everything yet, but will in the upcoming days.
status-firefox51: ? → affected
Flags: needinfo?(ahalberstadt)
Version: 52 Branch → 51 Branch
For the record because I may have not made this clear..

But leak checking itself never broke, just the jobs turning orange did. So it's easy to tell if a particular leak exists on an older branch (51 or 52) just by looking at the treeherder failure summary for the corresponding job (just beware the chunks are probably different).

Updated

3 months ago
Depends on: 1326456
Nothing needs to be whitelisted for 51:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05afd08fcc834310ad57c8f643028d5b07fb40a4

Landed here:
https://hg.mozilla.org/releases/mozilla-beta/rev/51167fd9b8e4d8a3449edd0aa08f95f3257d5469

I'm still waiting for try results to adjust the whitelist accordingly for aurora.
status-firefox51: affected → fixed
status-firefox53: affected → fixed

Updated

3 months ago
Depends on: 1328590
The try run for aurora looks good too, I pared the whitelist down to the minimum necessary:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d230bb3c68d71fff39f2136af394e327ba74a175

Landed all three patches here:
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c6b325d419763fef5172fdbd06e500f023a428e
https://hg.mozilla.org/releases/mozilla-aurora/rev/7bdc0736906babab2e5f01f394859202da71b60a
https://hg.mozilla.org/releases/mozilla-aurora/rev/05a3443d658d29e77cae1da1915ca51814534d73
status-firefox52: affected → fixed
No need to nominate test-only changes. I think we're done for this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
tracking-firefox51: ? → ---
tracking-firefox52: ? → ---
Resolution: --- → FIXED
Depends on: 1317290
Depends on: 1326136
https://treeherder.mozilla.org/#/jobs?repo=try&revision=962244b79fd5
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d048b1725de
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b8d127e8451
Blocks: 1333049
You need to log in before you can comment on or make changes to this bug.