344403 - Don't refer to |this| in accessible object constructors

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

It is dangerous to call any method which refers to |this|, because the vtbl is not fully constructed yet. Also the object isn't refcounted yet, so if using this causes it to get addrefed and released you will be destroyed during construction.

Bad practice. It can lead to crashes.

Comment 1

[Aaron Leventhal]

*** Bug 344402 has been marked as a duplicate of this bug. ***

Comment 2

[Aaron Leventhal]

Attachment: Remove Init()'s from constructors

Comment 3

[Mats Palmgren (inactive)]

+  accService->CreateHyperTextAccessible(captionNode, aCaption);
+  nsCOMPtr<nsPIAccessNode> accessNode(do_QueryInterface(*aCaption));
+  if (accessNode) {
+    accessNode->Init();
+  }
 }

You're falling off the edge without returning a value.
CreateHyperTextAccessible() sounds like it can fail?


+    nsCOMPtr<nsPIAccessNode> accessNode(do_QueryInterface(accHead));
+    accessNode->Init();

Missing "if (accessNode)" here?

Init() can fail?

Comment 4

[Aaron Leventhal]

Attachment: Fix error condition handling

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 229027 [details] [diff] [review]
Fix error condition handling

Looks good, r=mats on what you have so far.
But, I searched for Init() under accessible/ and found more...

Propagate Init() return value here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLSelectAccessible.cpp#822
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLSelectAccessible.cpp#888
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLSelectAccessible.cpp#973
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessNode.cpp#367
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsAccessibilityService.cpp#365
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLSelectAccessible.cpp#1225

You should probably check the Init() return value and take action here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLImageAccessible.cpp#161
http://lxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLTextAccessible.cpp#223
http://lxr.mozilla.org/seamonkey/source/accessible/src/msaa/nsAccessNodeWrap.cpp#409

You have a remaining Init() of |this| in the constructor here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsCaretAccessible.cpp#61

Comment 6

[Aaron Leventhal]

Neil, I accidentally wrote sr=neil on the checkin comment. Sorry about that.

Mats, I think we're going to have to generally go through and check return values better in a code cleanup at some point.