291077 - [crash][FIX]get_accParent() jumps to top level window object too soon, and loops, causing crashes

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

We are supposed to report the ROLE_WINDOW object in the get_accParent()
hierarchy whenever the current object was displayed in a new HWND. However, we
are sometimes doing that for get_accParent() on a child of the new object in the
window. This is 1 level in the hierarchy too soon.

Comment 1

[Aaron Leventhal]

Attachment: Change get_accParent() algorithm to get window accessible when the current frame has a window. Get the parent window with the scrollbars if there are two windows used.

Whichever of you can get to this can you r+sr= it? It's causing crashes in both
screen readers which has pretty much halted beta testing.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not a good reviewer for this patch -- I don't understand anything about what
it's doing or what the surrounding code is doing.

Comment 3

[Aaron Leventhal]

This code gets the accessible parent in 2 ways:
1. If the current object is at the root of its own HWND, we return the operating
system's generated accessible for that HWND. Howver, sometimes there are 2 HWNDs
-- 1 containing scrollbars and 1 child of that containing just the client area.
In that case we want to get the accessible from the parent of the two HWNDs,
otherwise get_accParent() will continually loop.

2. In all other cases we just return an accesible as a result of our own
nsIAccessible::GetParent()

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 192733 [details] [diff] [review]
Change get_accParent() algorithm to get window accessible when the current frame has a window. Get the parent window with the scrollbars if there are two windows used.

r+sr=jst FWIW.

Comment 5

[Aaron Leventhal]

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

Comment 6

[Aaron Leventhal]

This patch fixes several crashes in both screen readers we support.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is this patch correct?

+        // If frame has a widget that means we're at the root
+        // of a document or application, where there are 2 windows, one
+        // containing the scrollbars and one for the client area inside

This certainly isn't true in general. For example, position:fixed elements
always have widgets but they don't fit this pattern.

+        // If a frame is a scrollable frame, then it has one window with the
scrollbars
+        nsIScrollableFrame *scrollFrame = nsnull;
+        CallQueryInterface(frame, &scrollFrame);
+        if (scrollFrame) {
+          hwnd =
(HWND)scrollFrame->GetScrolledFrame()->GetWindow()->GetNativeData(NS_NATIVE_WINDOW);

This gets the "scrollport" HWND that doesn't have scrollbars...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I talked to Aaron on IRC and he convinced me that although the comments are
incorrect, the code is OK. I think he should fix the comments though.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Then I changed my mind and he's going to do a new patch.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 192733 [details] [diff] [review]
Change get_accParent() algorithm to get window accessible when the current frame has a window. Get the parent window with the scrollbars if there are two windows used.

I sic'ed roc on this, so I'll note the desire for a new patch by undoing
cbeard's +.

/be

Comment 11

[Aaron Leventhal]

Attachment: New version of get_accParent() -- cvs diff won't give me permission right now

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 193101 [details]
New version of get_accParent() -- cvs diff won't give me permission right now

this looks considerably more sensible.

Comment 13

[Jay Patel [:jay]]

aaronl:  Is there a testcase for this or steps to reproduce the crash?  Just
wanted to verify the fix with the latest builds...so if there is a way to test
this, let me know.  Or feel free to verify on both the trunk and branch
yourself. Thanks!

Comment 14

[Aaron Leventhal]

(In reply to comment #13)
> aaronl:  Is there a testcase for this or steps to reproduce the crash?  Just
> wanted to verify the fix with the latest builds...so if there is a way to test
> this, let me know.  Or feel free to verify on both the trunk and branch
> yourself. Thanks!

Jay, it's not necessary. We've had two separate communities of screen reader
testers plus the IBM accessibility center pounding on this stuff for months now.

If you want, you can download a beta demo of Window-Eyes from GW Micro at
http://www.gwmicro.com and open a view source window and then close it, and
reopen it to see if that crashes. Also, help -> help contents was another
similar place.

Comment 15

[Jay Patel [:jay]]

v.fixed on branch and trunk.