726514 - Blacklist test_process_error.xul in leak analysis

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Phil Ringnalda (:philor)]

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.

Comment 1

[Serge Gautherie (:sgautherie)]

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)
}

Comment 2

[Edmund Wong (:ewong)]

Attachment: Do not print out test_process_error.xul in leak analysis. (v1)

Comment 3

[Ed Morley [:emorley]]

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 :-)

Comment 4

[Phil Ringnalda (:philor)]

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

[Ed Morley [:emorley]]

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.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Blacklist test_process_error.xul in leak analysis. (v2)

Comment 7

[Ed Morley [:emorley]]

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+ :-)

Comment 8

[Edmund Wong (:ewong)]

Attachment: Blacklist test_process_error.xul in leak analysis (v3)

Comment 9

[Ed Morley [:emorley]]

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! :-)