The default bug view has changed. See this FAQ.
Bug 376924 (a11yframecrash)

Traversing the accessible tree after changes to CSS display property can crash Firefox

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Lukas Loehrer, Assigned: Aaron Leventhal)

Tracking

(Blocks: 1 bug, 5 keywords)

unspecified
access, crash, testcase, verified1.8.0.13, verified1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.4 -
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.12 -
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] destroyed frame)

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Emacs-w3m/1.4.4 w3m/0.5.1+cvs-1.968
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a4pre) Gecko/20070408 Minefield/3.0a4pre

The attached sample page shows and hides a region by setting its CSS
"display" property to "block" or "none". If the accessible tree is
traversed after a change from "block" to "none", Firefox under Linux
crashes. The attached javascript shell log shows the required steps.

The talkback report for this problem is 31019831.

On Windows with the exact same Minefield build (2007-04-08),
the problem does not occur in this simple case. I will attach a
comment for how to provoke a similar crash on Windows.



Reproducible: Always

Steps to Reproduce:
See the attached javascript shell log
Actual Results:  
Firefox crashes

Expected Results:  
The accessible tree should reflect the changes to the "display"
property and Firefox should not crash when the accessible tree is
traversed. 

Some information about the traversal of the accessibel tree:

The code used can be found in:

http://firemacs.googlecode.com/svn/trunk/tests/firefox/moz.js

The relevant functions are "dumpAccessibleTree" and
"dumpAccessibleNode".

The first function will simply traverse the accessible tree
recursively, calling the second fuction for each node.
dumpAccessibleNode will access finalRole, name, finalValue, state, extState and
description for each node. These accesses sometimes throw an
exception which does not make much sense to me. For provoking the
crash, it is apparently important to actually access these node
properties, i.e. if the line calling dumpAccessibleNode is commented
out in dumpAccessibleTre, no crash occurs. In other words, simply
walking the tree structure is not enough for a crash.
(Reporter)

Comment 1

10 years ago
Created attachment 261028 [details]
Test page for this bug. Requires display.js

this test case should work as follows:

Hitting the link should show or hide a line of text with yellow
backgroundd. This is achieved via the CSS display property. If the
checkbox is checked, the script will also make a spurious update to
the structure of the DOM in order to force an EVENT_REORDER event for
the node just above the element that has its "display" property
changed.
(Reporter)

Comment 2

10 years ago
Created attachment 261029 [details]
This is the javascript file needed by  the previous attachment
(Reporter)

Comment 3

10 years ago
Created attachment 261030 [details]
Shows the required steps to produce the crash
(Reporter)

Comment 4

10 years ago
Created attachment 261034 [details]
Javascript shell log for provoking a probably related crash on win32

While the Windows version appears to be more resilient, it can also be
crashed or at least confused with a more complicated example: The
comments page on blogger offers the option to show or hide the
original post. This effect is achieved with the CSS display property.
Example:

https://www2.blogger.com/comment.g?blogID=32049549&postID=503788244167924508

The link "Show original post" is what we are interested here.

With jaws, showing the post works fine, but after hitting the
link a second time to hide the post again, jaws is completely confused. When
using a remote javascript shell to click the link programmatically, FF
wil crash after the second click, which should hide the original post
again; this is without any AT running on Windows.

See the attachment for the required steps.

Again, it is important to actually access the properties of the
accessibel nodes while traversing the tree in order to provoke the
crash. The next step should probably be to find out the exact property of
the accessible node whose access leads to the crash.
(Assignee)

Updated

10 years ago
Assignee: nobody → aaronleventhal
Component: Disability Access → Disability Access APIs
Keywords: access, crash
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
(Assignee)

Updated

10 years ago
Blocks: 343213, 368881

Comment 5

10 years ago
We're using a destroyed frame.  Branches are affected too I think.
Group: security
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All

Comment 6

10 years ago
The problem is the cached frame pointers in nsHTMLTextAccessible,
nsHTMLLinkAccessible (and on trunk, nsHTMLListBulletAccessible).
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/html/nsHTMLTextAccessible.h&rev=1.47&root=/cvsroot&mark=68-71#48

AFAICT we notify frame changes from two places:
nsCSSFrameConstructor::RecreateFramesForContent
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1336&root=/cvsroot&mark=11115-11134#11059

and nsFrameManager::ReParentStyleContext
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsFrameManager.cpp&rev=1.245&root=/cvsroot&mark=1374-1389#1307

but only when "shell->IsAccessibilityActive()", which requires an
NS_ACCESSIBLE_EVENT to be activated:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsPresShell.cpp&rev=3.984&root=/cvsroot&mark=5835,5815#5810

The crash occurs because the we never got such an event and hence
the accessible nodes we're not notified of frame changes.

Comment 7

10 years ago
Created attachment 261328 [details] [diff] [review]
Patch rev. 1

This fixes the crash and makes the code slightly more robust:
  1. make the shell to be IsAccessibilityActive if not done already
     in nsAccessibilityService::GetAccessible()
  2. check IsAccessibilityActive() in GetFrame() and invalidate
     the cached frame if it's false (since we may have missed events)
  3. always invalidate the cached frame in FireToolkitEvent, not just
     for EVENT_HIDE. I'm think we can have a destroyed frame also for
     EVENT_REORDER (when frame != newFrame in RecreateFramesForContent -
     see nsCSSFrameConstructor.cpp link above).
Attachment #261328 - Flags: review?(aaronleventhal)

Comment 8

10 years ago
I suspect there are other cases where frames are replaced or destroyed that
does not go through RecreateFramesForContent/ReParentStyleContext so I
think we should try to remove this caching of raw frame pointers if we can.
Do we cache these frames for performance only? 
If so, is it still needed? (on trunk)
Can we solve it differently? (in the frame manager perhaps?)
Actually, for primary frames we should always go through one or the other...

That said, why are we doing _anything_ for the DOM mutation if accessibility is not active??
(In reply to comment #9)
> Actually, for primary frames we should always go through one or the other...

I was suspecting the code that creates first-letter frames, but I haven't
checked it closely, maybe you're right that it's covered...

> That said, why are we doing _anything_ for the DOM mutation if
> accessibility is not active??

Good point, but I think the problem here is that accessibility
should have been activated.  I think the intention is to activate
it for the shell "on demand" to avoid frame notifications when a11y
isn't used, but I could be wrong...
> I was suspecting the code that creates first-letter frames

Hmm....  those can indeed go away and reappear as needed, you're right.
(Assignee)

Comment 12

10 years ago
Bug 376884 might be a duplicate of this.

> Do we cache these frames for performance only? 
Yes, we cache them  because GetPrimaryFrameFor() must add extra items to primary frame hashtable whenever request comes in for a frame. Since we already have the frame when accessible is created it would seem to save that work. However, it may
have been a premature optimization and not really be a big improvement. It seems that getting rid of that optimization (caching the frames) is wise, to avoid the  crashes. That's something that can be done for branch as well.
(Assignee)

Comment 13

10 years ago
Accessibility should be active for the whole app or not at all. If  accessibility is not active, the accessibility service should not be getting called. I think presShell should just use a static global gIsAccessibilityActive, rather than a per-preshell mIsAccessibilityActive.
(Assignee)

Comment 14

10 years ago
Well, for list bullet frame it's not a performance enhancement. Caching the list bullet frame is necessary to cache otherwise we don't know how to get it.
(Assignee)

Comment 15

10 years ago
Created attachment 261333 [details] [diff] [review]
Remove caching of frames in accessibility

This should also fix this bug.

- We'll need to file a follow-up bug to deal with list bullet bounds and other issues. We already have many issues with list bullets because the list bullet text is not stored in anonymous content. Since there is no content node many of our normal methods don't work.
Attachment #261333 - Flags: review?(mats.palmgren)
(Assignee)

Comment 16

10 years ago
Comment on attachment 261328 [details] [diff] [review]
Patch rev. 1

+    nsAccessibleEvent event(PR_TRUE, NS_GETACCESSIBLE, nsnull);
+    nsEventStatus status;
+    mIgnoreShellGetAccessible = 1; // protect against recursive calls
+    aPresShell->HandleEventWithTarget(&event, nsnull, nsnull, &status);
+    mIgnoreShellGetAccessible = 0;
+  }

So I guess the presShell doesn't know that accessibility is active because it's being activated by Javascript instead of the normal OS way (via NS_GETACCESSIBLE).

Rather than doing this, I think we should have nsIPresShell::SetAccessibilityActive(PRBool);
and have that be a global static instead of a member variable.
Attachment #261328 - Flags: review?(aaronleventhal) → review-
(Assignee)

Comment 17

10 years ago
Lukas, is JS required to get crashes with these mutations? Or can you get it to happen just when using Firefox with MSAA or ATK?
(In reply to comment #16)
> Rather than doing this, I think we should have
> nsIPresShell::SetAccessibilityActive(PRBool);

Can't do that change on branches though.

> and have that be a global static instead of a member variable.

I don't see a problem with having a bit per shell if we want to.
Not sure how much of a performance win that is though.
Comment on attachment 261333 [details] [diff] [review]
Remove caching of frames in accessibility

Looks good.  r=mats

We still need to flip that mIsAccessibilityActive bit if we want these
notifications.  Not sure if that part is important enough for branch though.
Attachment #261333 - Flags: review?(mats.palmgren) → review+
(Reporter)

Comment 20

10 years ago
> Lukas, is JS required to get crashes with these mutations? Or can you get it to
> happen just when using Firefox with MSAA or ATK?

A crash can also be provoked just by using orca or lsr to interact with
Firefox. I am still refering to the April 8 version. Should I try with
the latest Minefield again? I do not know if the crash can also be
provoked on Windows without graversing the accessible tree in
javascript.

Also, I do not think that the crash does depend on the fact that no
other screen reader is running. If it helps, I can produce talkback reports
with jaws or orca running. On Linux, when I start Firefox, it will say
on the console that accessibility support is enabled regardless
whether orca is running.

Moreover, I think what makes these crashes show more predictively with my
AT (Firemacs) than with other ATs on Linux is that I use a virtual
buffer and thus have to traverse the whole accessable tree in one go, reading
at least "name" and "finalRole" for each node. The other Linux ATs
like lsr or orca seem to access a node onle when the user moves to it
on the page.
(Reporter)

Comment 21

10 years ago
Elaborating on my previous comment, here are the steps to provoke a
crash of Minefield April 8 with orca. I tried the version of April 12,
but it crashes even on startup:

1. Load page e.g.:
http://homepage.hispeed.ch/loehrer/firemacs/tests/display.html
(This is the same as the attachment above)

2. Visit  all the lines on the page with the screen reader. This step
   seems important, probably to trigger the creation of all the nodes
   in the accessible tree.

3. Hit the link "Hide region"

4. move the cursor down one line -> Feedback Agent pops up

Not sure if the last step is needed. I cannot really tell when the
Agent pops up exactly because I cannot see the screen at all. I can
get sighted assistance if more precise information is needed.

Created attachment 261377 [details]
Standalone testcase for trunk, CRASH on load

Updated

10 years ago
Flags: in-testsuite?
Keywords: testcase
Whiteboard: [sg:critical?] destroyed frame
(Assignee)

Comment 23

10 years ago
Spun off bug 377288 for issue on getting accessibility notifications even if the a11y service is launched from JS. That bug isn't security sensitive or a crasher.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

10 years ago
We should let this bake in and then consider it for the 1.8 branch. How should we mark it up so we remember to do that in a week or so. Is there a way to mark up the patch as potentially needed for 1.8, without sending it for a= right away?
(Assignee)

Updated

10 years ago
Duplicate of this bug: 376884
Mats's testcase in comment 22 crashes FF2.0.0.4pre. Nominating for the branch.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
I think we want more baking that we can get for 1.8.1.4 -- not blocking this release but marked "wanted" for the branch so we remember to come back to it.
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4-
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12-
(Assignee)

Updated

10 years ago
Blocks: 333488
(Assignee)

Comment 28

10 years ago
The machine I had with an older version of Visual C++ went down, and the older branches don't build with VC8. Can I get any help from Mozilla to backport this fix?
(Assignee)

Updated

10 years ago
Alias: a11yframecrash
Created attachment 265073 [details] [diff] [review]
Patch for branches
Attachment #261328 - Attachment is obsolete: true
Attachment #265073 - Flags: approval1.8.1.5?
Attachment #265073 - Flags: approval1.8.0.13?
Created attachment 265075 [details]
stack, branch 1.8

FWIW, the "Standalone testcase" still crash on branch with the attached
branch patch, but I think it's an unrelated (null-ptr) crash.
(Assignee)

Comment 31

10 years ago
Mats, how should we deal with the fact that it still crashes?
As a separate unrelated bug IMO.  I looked briefly at the code and it
seemed like a null-ptr crash only.  compare 1.8 branch:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/atk/nsAccessibleHyperText.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=184,190#182

with trunk:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/html/nsHyperTextAccessible.cpp&rev=1.58&root=/cvsroot&mark=1378#1375

Since the code appears to be so different I'm not sure if it's worth the
effort+risk of rewriting the code to much for a null-ptr crash...
we could try to wallpaper it with an early return I guess...
(Assignee)

Comment 33

10 years ago
Ah. That's a Linux-only bug then, whereas this bug is for all operating systems. Our accessibility support for Linux on branch is really different from what we have now. It's not worth doing much to fix it. Wallpaper is fine.
Spawned off bug 380975 for the additional branch null-ptr crash.
Flags: blocking1.8.1.5+
Comment on attachment 265073 [details] [diff] [review]
Patch for branches

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Attachment #265073 - Flags: approval1.8.1.5?
Attachment #265073 - Flags: approval1.8.1.5+
Attachment #265073 - Flags: approval1.8.0.13?
Attachment #265073 - Flags: approval1.8.0.13+
Comment on attachment 265073 [details] [diff] [review]
Patch for branches

I landed the branch patch on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH
at 2007-07-01 15:46 PDT.

Updated

10 years ago
Keywords: fixed1.8.0.13, fixed1.8.1.5
verified fixed 1.8.1.5 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/20070705 BonEcho/2.0.0.5pre ID:2007070504 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.5pre) Gecko/2007070503 BonEcho/2.0.0.5pre on Linux Fedora F7. No crash on the testcase from mats (comment #22). Adding verified keyword
Keywords: fixed1.8.1.5 → verified1.8.1.5
verified fixed 1.8.0.13 using the branch testcase in this bug from mats (comment #22) and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.13pre) Gecko/20070822 Firefox/1.5.0.13pre 

No crash on testcase - adding verified keyword
Keywords: fixed1.8.0.13 → verified1.8.0.13
Group: security
You need to log in before you can comment on or make changes to this bug.