Closed Bug 1325148 Opened 8 years ago Closed 8 years ago

Mozleak should use the error level instead of the warning level

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

(Keywords: regression, Whiteboard: [leave-open])

Attachments

(3 files, 4 obsolete files)

Bug 1325148 - Use "error" level in mozleak when logging leak failures,
59 bytes, text/x-review-board-request
jgraham
: review+
Details
Bug 1325148 - Temporarily disable mochitest leakchecking for directories containing known leaks,
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1325148 - Temporarily disable leakchecking in crashtests on linux,
59 bytes, text/x-review-board-request
jmaher
: review+
Details
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.
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 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.
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 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 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 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.
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
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.
Attachment #8820841 - Attachment is obsolete: true
Attachment #8821731 - Flags: review?(ahalberstadt)
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.
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+
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
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
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.
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)
I also documented the c3 leak here a bit more in bug 1325947
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
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.
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).
Depends on: 1326456
No need to nominate test-only changes. I think we're done for this bug.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
tracking-firefox51: ? → ---
tracking-firefox52: ? → ---
Resolution: --- → FIXED
Depends on: 1317290
Depends on: 1326136
Blocks: 1333049
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: