Mozleak should use the error level instead of the warning level

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
2 years 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])

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
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.
Depends on: 1325149
Depends on: 1325158
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
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

2 years 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+
(Assignee)

Updated

2 years ago
Depends on: 1325215
(Assignee)

Updated

2 years ago
Depends on: 1325274
(Assignee)

Updated

2 years ago
Depends on: 1325275
(Assignee)

Updated

2 years ago
Depends on: 1325277
Whitelisted bugs should have highest Importance because they would have caused backout if mozleak had worked.
(Assignee)

Comment 5

2 years ago
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)
(Assignee)

Comment 9

2 years ago
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

2 years 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

2 years 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

2 years 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.
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.
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)
(Assignee)

Comment 31

2 years ago
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+
(Assignee)

Updated

2 years ago
Attachment #8821872 - Flags: review?(ahalberstadt) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 35

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8821731 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8821872 - Attachment is obsolete: true

Comment 36

2 years 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)
(Assignee)

Updated

2 years ago
Attachment #8822024 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 38

2 years ago
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.
(Assignee)

Comment 41

2 years ago
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)
(Assignee)

Comment 47

2 years ago
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

2 years 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
(Assignee)

Comment 50

2 years ago
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.
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
(Assignee)

Updated

2 years ago
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.
(Assignee)

Comment 56

2 years ago
(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
(Assignee)

Comment 57

2 years ago
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

2 years ago
Depends on: 1326456
(Assignee)

Comment 58

2 years ago
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
Depends on: 1328590
(Assignee)

Updated

2 years ago
status-firefox52: affected → fixed
(Assignee)

Comment 60

2 years ago
No need to nominate test-only changes. I think we're done for this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
tracking-firefox51: ? → ---
tracking-firefox52: ? → ---
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Blocks: 1333049
You need to log in before you can comment on or make changes to this bug.