333134 - Accessibility still leaking when AT used

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The accessibility module is still leaking. 

Steps:
1. Set XPCOM_MEM_LEAK_LOG=1
2. Load about:blank in firefox.exe or mfcembed.exe 
3. Run over it with Accessible Object Explorer
4. Exit Accessible Object Explorer
5. Exit browser

Here's what you get in the leak log.

== BloatView: ALL (cumulative) LEAK STATISTICS

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          37      920    34285       17 (  189.85 +/-   270.66)   107118      168 (   54.96 +/-    94.03)
   2 BackstagePass                                  20       20        1        1 (    1.00 +/-     0.00)      299        3 (   56.32 +/-    31.28)
  63 XPCNativeScriptableShared                     108      216      202        2 (    8.33 +/-     2.42)        0        0 (    0.00 +/-     0.00)
  65 XPCWrappedNative                               56      224      256        4 (  120.93 +/-    70.76)     1392        4 (  140.32 +/-    75.80)
  82 nsAccessNode                                   28       28        1        1 (    1.00 +/-     0.00)      980       32 (   25.34 +/-    10.21)
  84 nsAccessible                                   60       60        1        1 (    1.00 +/-     0.00)      980       32 (   25.34 +/-    10.21)
  99 nsBlockAccessible                              76       76        1        1 (    1.00 +/-     0.00)      980       32 (   25.34 +/-    10.21)
 190 nsDocAccessible                               164      164        1        1 (    1.00 +/-     0.00)      980       32 (   25.34 +/-    10.21)
 315 nsRect                                         16       32    10506        2 (   94.55 +/-    12.51)        0        0 (    0.00 +/-     0.00)
 319 nsRootAccessible                              196        0        0        0 (    0.00 +/-     0.00)      978       31 (   24.38 +/-    10.18)
 361 nsSystemPrincipal                              36       36        1        1 (    1.00 +/-     0.00)       24        1 (    3.96 +/-     1.44)
 381 nsVoidArray                                     4        8     1794        2 (  403.44 +/-   136.71)        0        0 (    0.00 +/-     0.00)
 407 nsXPCComponents                                56       56       63        1 (   31.75 +/-    18.12)      695        1 (  112.42 +/-    63.74)

Comment 1

[Aaron Leventhal]

Attachment: Same leaks as above, more readable (accessible event handling is turned off)

I commented out nsRootAccessible::AddEventListener/RemoveEventListener before creating this report. So it's not our event listening.

Comment 2

[Dan Kinnunen]

Aaron, let me know when you want me to run my "mem leak perl scripts" against the fix.

Comment 3

[Aaron Leventhal]

Attachment: Fix 2 more leaks

This improves things quite a bit, but some AT's are still leaking with us:

Does it cause Mozilla leaks?
MSAA Inspect.exe -- yes
Window-Eyes -- no
MSAA AccExplore.exe -- yes (0 objects leak but 1 refcount ... strange)

However I think we should get this in and then run more tests afterwards.

Comment 4

[Ginn Chen]

Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

You added extra {} in the first part of nsWindow.cpp (Lines 4754-4766, outside if)

Comment 5

[David Baron :dbaron:]

Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

>-      if (nsWindow::gIsAccessibilityOn) {
>-        // Create it for the first time so that it can start firing events
>-        GetRootAccessible();
>+      {
>+        if (nsWindow::gIsAccessibilityOn) {
>+          // Create it for the first time so that it can start firing events
>+          nsCOMPtr<nsIAccessible> rootAccessible = GetRootAccessible();
>+        }

I don't see any need for the extra braces/indenting here, but otherwise sr=dbaron.

Comment 6

[Ginn Chen]

Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

I can approve after Bug 334556 is solved.

Comment 7

[Ginn Chen]

committed with patch of Bug 334556

Checking in accessible/src/base/nsDocAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.73.2.8; previous revision: 1.73.2.7
done
Checking in widget/src/windows/nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.569.2.17; previous revision: 3.569.2.16
done
Checking in widget/src/windows/nsWindow.h;
/cvsroot/mozilla/widget/src/windows/nsWindow.h,v  <--  nsWindow.h
new revision: 3.204.2.10; previous revision: 3.204.2.9
done
Checking in accessible/src/atk/nsRootAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsRootAccessibleWrap.cpp,v  <--  nsRootAccessibleWrap.cpp
new revision: 1.5.44.2; previous revision: 1.5.44.1
done
Checking in accessible/src/atk/nsRootAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/atk/nsRootAccessibleWrap.h,v  <--  nsRootAccessibleWrap.h
new revision: 1.5.44.2; previous revision: 1.5.44.1
done

Comment 8

[Aaron Leventhal]

Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

This sister to this bug is bug 334556, which is necessary to go in at the same time.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

approved for 1.8.0 branch, a=dveditz for drivers

Comment 10

[David Baron :dbaron:]

It seems a lot safer to me to skip the nsDocAccessible.cpp patch, and bug 334556, for 1.8.0.3.

Comment 11

[David Baron :dbaron:]

(Or is there some non-obvious relationship between the nsDocAccessible.cpp changes and the other changes?)

Comment 12

[Aaron Leventhal]

(In reply to comment #11)
> (Or is there some non-obvious relationship between the nsDocAccessible.cpp
> changes and the other changes?)

Okay, we can do that if the leak report it fixes is not real and you think it's safer. On the other hand, I probably don't understand all of the consequences of adding |this| to our cache before the vtbl is full. Are you sure that we're not fixing some other bugs or crashes by doing this?

Comment 13

[David Baron :dbaron:]

(In reply to comment #12)
> On the other hand, I probably don't understand all of the consequences
> of adding |this| to our cache before the vtbl is full.

The consequence is that an object that is *going* to be a derived class has vtable pointers indicating that it's the base class (nsDocAccessible, in this case) until the constructor of each of those derived classes finishes with the base class initialization phase of the constructor.

> Are you sure that we're
> not fixing some other bugs or crashes by doing this?

No, but I'm also not sure we're not causing other bugs or crashes due to the change in refcounting, given the amount of stuff that's still done in the constructors.

Comment 14

[Ginn Chen]

widget/src/windows part committed

Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.569.2.6.2.1; previous revision: 3.569.2.6
done
Checking in nsWindow.h;
/cvsroot/mozilla/widget/src/windows/nsWindow.h,v  <--  nsWindow.h
new revision: 3.204.2.4.2.1; previous revision: 3.204.2.4
done