The default bug view has changed. See this FAQ.

Accessibility still leaking when AT used

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

(4 keywords)

Trunk
x86
All
access, fixed1.8.0.4, fixed1.8.1, mlk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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)
(Assignee)

Comment 1

11 years ago
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.
(Assignee)

Updated

11 years ago
Attachment #217562 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

11 years ago
Attachment #217562 - Attachment description: Leaks (with accessible event handling turned off) → Same leaks as above, more readable (accessible event handling is turned off)

Updated

11 years ago
Keywords: mlk
(Assignee)

Updated

11 years ago
Blocks: 333488

Comment 2

11 years ago
Aaron, let me know when you want me to run my "mem leak perl scripts" against the fix.
(Assignee)

Comment 3

11 years ago
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.
Attachment #218457 - Flags: review?(ginn.chen)

Comment 4

11 years ago
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+
(Assignee)

Updated

11 years ago
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+
(Assignee)

Updated

11 years ago
Attachment #218457 - Flags: approval-branch-1.8.1?(ginn.chen)
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Depends on: 334556

Comment 6

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

I can approve after Bug 334556 is solved.

Updated

11 years ago
Attachment #218457 - Flags: approval-branch-1.8.1?(ginn.chen) → approval-branch-1.8.1+

Comment 7

11 years ago
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
(Assignee)

Comment 8

11 years ago
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?)
(Assignee)

Comment 12

11 years ago
(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.

Comment 14

11 years ago
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.