300833 - Increased leakage on balsa from 290354

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P1)

Description

[Caleb]

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1121350680&maxdate=1121354639

There were 2 checkins in that timeframe, and since bug 290354 caused some Tp
regressions, it might be the cause of the increased leakage aswell.

From:
RLk:3.58KB
Lk:297KB
MH:7.67MB
A:215K

To:
RLk:4.98KB
Lk:300KB
MH:7.78MB
A:219K

Comment 1

[Boris Zbarsky [:bzbarsky]]

The log says:

--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsGenericElement                                700          -
nsNodeInfoManager                                40          -
nsHashtable                                      88          -
nsEventListenerManager                         1100          -
nsPrincipal                                     240          -
nsGenericDOMDataNode                            644          -
nsCSSDeclaration                                 56          -
nsDOMEventGroup                                  12          -
nsCSSRule                                        40          -
nsNodeInfo                                       64          -
nsStandardURL                                   368     100.00%
nsLocalFile                                     720      20.00%
TOTAL                                          4072

Comment 2

[Asa Dotzler [:asa]]

Aaron, time is getting short for 1.8b4. Can you please look into this quickly?

Comment 3

[Aaron Leventhal]

I suspect the fix for bug 300783 will take care of this as well.

Comment 4

[Caleb]

Yeah, this got fixed by bug 300783.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, no.  The checkin for bug 290354 might have fixed some leaks, but by no
means all.  If nothing else, the following line in
nsListControlFrame::FireMenuItemActiveEventleaks:

2649   nsCOMPtr<nsIContent> optionContent = GetOptionContent(focusedIndex);

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: This should fix it, maybe...

Needs careful testing (I've already spent several hours on this, and that's all
the time I'll be able to spend on it for the foreseeable future).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, I meant checkin for bug 300783. The checkin for bug 290354 is what caused
this, of course....

Comment 8

[Corinne Bordelon]

Blocking for 1.8b4 since it's a regression. Need some QA on this after the patch
lands, per Comment 6. Boris -> do you need a review prior to landing?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, this needs review.  It also needs at least minimal testing to make sure it
fixes the leak and that things still work before said review is requested.  I've
done no testing past making sure it compiles.

Comment 10

[Aaron Leventhal]

(In reply to comment #9)
> Yes, this needs review.  It also needs at least minimal testing to make sure it
> fixes the leak and that things still work before said review is requested.  I've
> done no testing past making sure it compiles.

With Boris' patch the correct accessibility events are still fired.

Comment 11

[Aaron Leventhal]

Comment on attachment 190625 [details] [diff] [review]
This should fix it, maybe...

Whichever one of you gets to this could you shoud r+sr= it since we're short on
time?

I've tested that it doesn't regress accessibility.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 190625 [details] [diff] [review]
This should fix it, maybe...

   nsIContent* content = nsnull;

This line is no longer needed, remove it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190625 [details] [diff] [review]
This should fix it, maybe...

Requesting approval for this leak fix.