334556 - crash on startup when GTK accessibility pref enabled

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I crash on startup when the GTK accessibility pref is enabled because the root accessible object is destroyed within its own constructor, at the stack to be attached.  This is in a trunk build from within the past few hours.

I suspect this is a regression from bug 333134 -- specifically the change to nsDocAccessible.cpp.  However, that change is correct -- you just need more of the same.  It's against the rules in Mozilla code to pass an object around from its own constructor, for two reasons:
 * you crash if something AddRefs and Releases it, since the reference count goes from 0 to 1 to 0 again
 * not all the vtable pointers are initialized, so strange things can happen (destructors have the same problem).  This is what confused the leak logging and caused the appearance of a negative leak that I'm guessing you were fixing in bug 333134's changes to nsDocAccessible.cpp.

I can see backing out that change temporarily to fix this, but in the long term you want to move forward by not passing |this| around from constructors or destructors.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: top of stack showing destructor called within constructor

Comment 2

[Aaron Leventhal]

Attachment: Untested

I'm not getting the crash myself, but this might fix the problem. David, want to test it?

Comment 3

[Ginn Chen]

Comment on attachment 218923 [details] [diff] [review]
Untested

It solved the crash, but firefox is not accessible. In at-poke, no accessible window under firefox application.

Backing out accessible/src/base/nsDocAccessible.cpp can solve this bug.

Comment 4

[Ginn Chen]

Attachment: patch v2

Comment 5

[Aaron Leventhal]

Comment on attachment 218956 [details] [diff] [review]
patch v2

Oh I forget the upcall, good catch. Very close.

Comment 6

[Aaron Leventhal]

Attachment: Upcall for nsRootAccessible::Init() & Shutdown()

Comment 7

[Aaron Leventhal]

I had accidentally posted this comment to bug 331120:

The only place in the constructor chain for nsRootAccessible that we pass
|this| around is here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/atk/nsRootAccessibleWrap.cpp#49

Ginn, I think we need to move that into nsRootAccessibleWrap::Init()

While we're at it the code in the destructor needs to be moved to the
Shutdown() method. Most things should happen in Shutdown() not in the
destructor because we're guaranteed that Shutdown() will be called when a DOM
Node is no longer needed. Unfortunately the refcount might not be 0 because a
poorly behaving accessibility user might not release when we want them to.

Looks like we'll have to go through the mozilla/accessible/atk and ensure
proper use of Init() and Shutdown() instead of constructor and destructor code.

Comment 8

[Ginn Chen]

Comment on attachment 218967 [details] [diff] [review]
Upcall for nsRootAccessible::Init() & Shutdown()

r=me

please don't change
-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+N/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

and we don't use rv so we don't need rv

+    nsresult rv = nsRootAccessible::Init();

Comment 9

[Aaron Leventhal]

Attachment: Don't forget to return rv

Comment 10

[Aaron Leventhal]

Comment on attachment 218970 [details] [diff] [review]
Don't forget to return rv

Ginn, does this build for you?

Comment 11

[Aaron Leventhal]

Comment on attachment 218970 [details] [diff] [review]
Don't forget to return rv

Verified that it builds and works.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

FWIW, bug 331120 comment 7 was meant for this bug.

Comment 13

[Ginn Chen]

Checking in src/atk/nsRootAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsRootAccessibleWrap.cpp,v  <--  nsRootAccessibleWrap.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in src/atk/nsRootAccessibleWrap.h;
/cvsroot/mozilla/accessible/src/atk/nsRootAccessibleWrap.h,v  <--  nsRootAccessibleWrap.h
new revision: 1.8; previous revision: 1.7
done

Comment 14

[Aaron Leventhal]

Comment on attachment 218970 [details] [diff] [review]
Don't forget to return rv

This has to go in at the same time as its sister, bug 333134.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 218970 [details] [diff] [review]
Don't forget to return rv

approved for 1.8.0 branch, a=dveditz for drivers

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems safer to me to skip this, and the part of the other patch that caused it, for 1.8.0.3.

Comment 17

[Aaron Leventhal]

Comment on attachment 218970 [details] [diff] [review]
Don't forget to return rv

Removing approval since for now we're deciding not to check this in, on dbaron's advice.