Closed Bug 333134 Opened 18 years ago Closed 18 years ago

Accessibility still leaking when AT used

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(4 keywords)

Attachments

(2 files)

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)
I commented out nsRootAccessible::AddEventListener/RemoveEventListener before creating this report. So it's not our event listening.
Attachment #217562 - Attachment mime type: application/octet-stream → text/plain
Attachment #217562 - Attachment description: Leaks (with accessible event handling turned off) → Same leaks as above, more readable (accessible event handling is turned off)
Keywords: mlk
Blocks: fox2access
Aaron, let me know when you want me to run my "mem leak perl scripts" against the fix.
Attached patch Fix 2 more leaksSplinter Review
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.
Attachment #218457 - Flags: review?(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)
Attachment #218457 - Flags: review?(ginn.chen) → review+
Attachment #218457 - Flags: superreview?(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.
Attachment #218457 - Flags: superreview?(dbaron) → superreview+
Attachment #218457 - Flags: approval-branch-1.8.1?(ginn.chen)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

I can approve after Bug 334556 is solved.
Attachment #218457 - Flags: approval-branch-1.8.1?(ginn.chen) → approval-branch-1.8.1+
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
Keywords: fixed1.8.1
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.
Attachment #218457 - Flags: approval1.8.0.3?
Comment on attachment 218457 [details] [diff] [review]
Fix 2 more leaks

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #218457 - Flags: approval1.8.0.3? → approval1.8.0.3+
It seems a lot safer to me to skip the nsDocAccessible.cpp patch, and bug 334556, for 1.8.0.3.
(Or is there some non-obvious relationship between the nsDocAccessible.cpp changes and the other changes?)
(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?
(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.
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
Keywords: fixed1.8.0.3
You need to log in before you can comment on or make changes to this bug.