If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED

Status

()

Core
Disability Access APIs
--
critical
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, crash, verified1.8})

Trunk
access, crash, verified1.8
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Updated

12 years ago
Keywords: crash
Summary: get_accParent() jumps to top level window object too soon → get_accParent() jumps to top level window object too soon, and loops, causing crashes
(Assignee)

Comment 1

12 years ago
Created 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.

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.
Attachment #192733 - Flags: superreview?(bzbarsky)
Attachment #192733 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Severity: normal → critical
Flags: blocking1.8b4?

Updated

12 years ago
Flags: blocking1.8b4? → blocking1.8b4+
(Assignee)

Updated

12 years ago
Summary: get_accParent() jumps to top level window object too soon, and loops, causing crashes → [crash][FIX]get_accParent() jumps to top level window object too soon, and loops, causing crashes
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.
(Assignee)

Updated

12 years ago
Attachment #192733 - Flags: superreview?(bzbarsky) → superreview?(dmose)
(Assignee)

Comment 3

12 years ago
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 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.
Attachment #192733 - Flags: superreview?(dmose)
Attachment #192733 - Flags: superreview+
Attachment #192733 - Flags: review?(roc)
Attachment #192733 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #192733 - Flags: approval1.8b4?
(Assignee)

Comment 5

12 years ago
*** Bug 304464 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

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

Updated

12 years ago
Attachment #192733 - Flags: approval1.8b4? → approval1.8b4+
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...
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.
Then I changed my mind and he's going to do a new patch.
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
Attachment #192733 - Flags: approval1.8b4+
(Assignee)

Comment 11

12 years ago
Created attachment 193101 [details]
New version of get_accParent() -- cvs diff won't give me permission right now
Attachment #193101 - Flags: superreview?(roc)
Attachment #193101 - Flags: review?(roc)
Comment on attachment 193101 [details]
New version of get_accParent() -- cvs diff won't give me permission right now

this looks considerably more sensible.
Attachment #193101 - Flags: superreview?(roc)
Attachment #193101 - Flags: superreview+
Attachment #193101 - Flags: review?(roc)
Attachment #193101 - Flags: review+
Attachment #193101 - Flags: approval1.8b4+
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED

Comment 13

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

Comment 14

12 years ago
(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.
Hardware: PC → All

Comment 15

12 years ago
v.fixed on branch and trunk.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8 → verified1.8
You need to log in before you can comment on or make changes to this bug.