Blacklist test_process_error.xul in leak analysis

RESOLVED FIXED

Status

Tree Management Graveyard
TBPL
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: philor, Assigned: ewong)

Tracking

Details

(Whiteboard: [sheriff-want])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Pretty much every mochitest-other leak analysis claims "chrome://mochitests/content/chrome/dom/ipc/tests/test_process_error.xul leaked 1 DOMWINDOW(s)" even when the leak was in some other sub-suite. It's never true, and it only leads to confusion and invalid bugs. We should just never output that line.
An example:

https://tbpl.mozilla.org/php/getParsedLog.php?id=9423652&tree=Firefox
Rev3 WINNT 6.1 mozilla-central debug test mochitest-other on 2012-02-17 12:28:06 PST for push eb85fbbeb6d9
{
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 448 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of CondVar with size 16 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of Mutex with size 12 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsCacheEntryHashTable with size 36 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of nsCacheService with size 148 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsDiskCacheBinding with size 52 bytes each (104 bytes total)
...
mochitest-browser-chrome: 26350/0/91 LEAK
}
+
https://tbpl.mozilla.org/php/getLeakAnalysis.php?id=9423652
{
chrome://mochitests/content/chrome/dom/ipc/tests/test_process_error.xul leaked 1 DOMWINDOW(s)
}
Severity: normal → major
(Reporter)

Updated

6 years ago
Severity: major → normal

Updated

6 years ago
Whiteboard: [sheriff-want]
(Assignee)

Comment 2

5 years ago
Created attachment 664837 [details] [diff] [review]
Do not print out test_process_error.xul in leak analysis. (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #664837 - Flags: review?(bmo)

Comment 3

5 years ago
philor, is the test_process_error.xul 'leak' an actual leak but caused by something else, or not actually a leak at all?

ie: if the former we should just s/test_process_error.xul/(unknown)/ like we do for "Shutdown"; or if the latter, we need to do as in the attached patch, but presumably handle the result == "" case too (if so, what string would you suggest for that?)

Lastly, bug 538462 seems to have been WFM for ages, so do you think we can remove the special casing for test_unknownContentType_dialog_layout.xul?

Cheers :-)
(Reporter)

Comment 4

5 years ago
It's not a leak at all, it just does something tricky so there's no --DOMWINDOW to see. The typical case is that we have a browser-chrome leak, mochitest-chrome didn't leak at all, and we show it as being the 2 domwindows that were leaked despite it being in a non-leaking suite.

I'd rather not remove the test_unknownContentType_dialog_layout.xul thing unless we replace it, since that's a reminder of what the page is supposed to show. We could stick in hints for bug 730746 and bug 787312 instead - neither one is all that frequent, but they're more frequent than 538462 :)

Comment 5

5 years ago
Comment on attachment 664837 [details] [diff] [review]
Do not print out test_process_error.xul in leak analysis. (v1)

Really sorry for the delay, last week was the end of the quarter so had a bunch of stuff get in the way.

AIUI, we need to cover the case where this was the only leak showing, but we've now hidden it & don't display anything. We should fall back to the "No DOMWINDOWs leaked!" case.

Also, if possible, would you mind replacing the test_unknownContentType_dialog_layout.xul case with the ones suggested by philor in comment 4.
Attachment #664837 - Flags: review?(bmo)
(Assignee)

Comment 6

5 years ago
Created attachment 669954 [details] [diff] [review]
Blacklist test_process_error.xul in leak analysis. (v2)
Attachment #664837 - Attachment is obsolete: true
Attachment #669954 - Flags: review?(bmo)

Comment 7

5 years ago
Comment on attachment 669954 [details] [diff] [review]
Blacklist test_process_error.xul in leak analysis. (v2)

Looks good, the only thing needing changing is that the |$result = "No DOMWINDOWs leaked!";| lost its <div>

With that fixed, I'll be happy to r+ :-)
Attachment #669954 - Flags: review?(bmo)
(Assignee)

Comment 8

5 years ago
Created attachment 672212 [details] [diff] [review]
Blacklist test_process_error.xul in leak analysis (v3)
Attachment #669954 - Attachment is obsolete: true
Attachment #672212 - Flags: review?(bmo)

Comment 9

5 years ago
I spotted one last change:
> if ($results == '')
needs to be:
> if ($result == '')

But I fixed that locally & testing with Vagrant looked good, so landed as:
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/26061ee21696

Thank you for the patch! :-)

Updated

5 years ago
Attachment #672212 - Flags: review?(bmo) → review+

Updated

5 years ago
Depends on: 804021

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.