Last Comment Bug 333134 - Accessibility still leaking when AT used
: Accessibility still leaking when AT used
Status: RESOLVED FIXED
: access, fixed1.8.0.4, fixed1.8.1, mlk
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Aaron Leventhal
:
Mentors:
Depends on: 334556
Blocks: fox2access
  Show dependency treegraph
 
Reported: 2006-04-07 07:38 PDT by Aaron Leventhal
Modified: 2006-04-23 22:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Same leaks as above, more readable (accessible event handling is turned off) (2.23 KB, text/plain)
2006-04-07 07:40 PDT, Aaron Leventhal
no flags Details
Fix 2 more leaks (5.08 KB, patch)
2006-04-14 12:02 PDT, Aaron Leventhal
ginn.chen: review+
dbaron: superreview+
ginn.chen: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description Aaron Leventhal 2006-04-07 07:38:59 PDT
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 2006-04-07 07:40:26 PDT
Created attachment 217562 [details]
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 2006-04-13 06:46:09 PDT
Aaron, let me know when you want me to run my "mem leak perl scripts" against the fix.
Comment 3 Aaron Leventhal 2006-04-14 12:02:52 PDT
Created attachment 218457 [details] [diff] [review]
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 2006-04-17 02:14:33 PDT
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: ⌚️UTC-7 (busy September 14-25) 2006-04-17 10:07:48 PDT
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 2006-04-18 20:33:59 PDT
Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

I can approve after Bug 334556 is solved.
Comment 7 Ginn Chen 2006-04-19 20:52:50 PDT
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 2006-04-19 21:31:15 PDT
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] 2006-04-21 14:27:25 PDT
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: ⌚️UTC-7 (busy September 14-25) 2006-04-21 14:33:24 PDT
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: ⌚️UTC-7 (busy September 14-25) 2006-04-21 14:34:27 PDT
(Or is there some non-obvious relationship between the nsDocAccessible.cpp changes and the other changes?)
Comment 12 Aaron Leventhal 2006-04-21 20:03:23 PDT
(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: ⌚️UTC-7 (busy September 14-25) 2006-04-21 23:36:44 PDT
(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 2006-04-23 22:40:16 PDT
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

Note You need to log in before you can comment on or make changes to this bug.