Last Comment Bug 376924 - (a11yframecrash) Traversing the accessible tree after changes to CSS display property can crash Firefox
(a11yframecrash)
: Traversing the accessible tree after changes to CSS display property can cras...
Status: RESOLVED FIXED
[sg:critical?] destroyed frame
: access, crash, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Aaron Leventhal
:
Mentors:
: 376884 (view as bug list)
Depends on:
Blocks: aria fox2access atk
  Show dependency treegraph
 
Reported: 2007-04-09 11:21 PDT by Lukas Loehrer
Modified: 2007-08-24 20:35 PDT (History)
9 users (show)
dveditz: blocking1.8.1.4-
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.12-
dveditz: wanted1.8.0.x+
mats: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test page for this bug. Requires display.js (715 bytes, text/html)
2007-04-09 11:34 PDT, Lukas Loehrer
no flags Details
This is the javascript file needed by the previous attachment (677 bytes, text/plain)
2007-04-09 11:36 PDT, Lukas Loehrer
no flags Details
Shows the required steps to produce the crash (5.41 KB, text/plain)
2007-04-09 11:43 PDT, Lukas Loehrer
no flags Details
Javascript shell log for provoking a probably related crash on win32 (1.01 KB, text/plain)
2007-04-09 12:02 PDT, Lukas Loehrer
no flags Details
Patch rev. 1 (9.06 KB, patch)
2007-04-11 18:12 PDT, Mats Palmgren (:mats)
aaronlev: review-
Details | Diff | Splinter Review
Remove caching of frames in accessibility (8.81 KB, patch)
2007-04-11 19:56 PDT, Aaron Leventhal
mats: review+
Details | Diff | Splinter Review
Standalone testcase for trunk, CRASH on load (2.06 KB, text/html)
2007-04-12 07:49 PDT, Mats Palmgren (:mats)
no flags Details
Patch for branches (7.06 KB, patch)
2007-05-16 18:12 PDT, Mats Palmgren (:mats)
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review
stack, branch 1.8 (2.67 KB, text/plain)
2007-05-16 18:16 PDT, Mats Palmgren (:mats)
no flags Details

Description Lukas Loehrer 2007-04-09 11:21:16 PDT
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.
Comment 1 Lukas Loehrer 2007-04-09 11:34:56 PDT
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.
Comment 2 Lukas Loehrer 2007-04-09 11:36:51 PDT
Created attachment 261029 [details]
This is the javascript file needed by  the previous attachment
Comment 3 Lukas Loehrer 2007-04-09 11:43:19 PDT
Created attachment 261030 [details]
Shows the required steps to produce the crash
Comment 4 Lukas Loehrer 2007-04-09 12:02:04 PDT
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.
Comment 5 Mats Palmgren (:mats) 2007-04-11 18:06:40 PDT
We're using a destroyed frame.  Branches are affected too I think.
Comment 6 Mats Palmgren (:mats) 2007-04-11 18:07:40 PDT
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 Mats Palmgren (:mats) 2007-04-11 18:12:46 PDT
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).
Comment 8 Mats Palmgren (:mats) 2007-04-11 18:14:18 PDT
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?)
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2007-04-11 18:21:07 PDT
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??
Comment 10 Mats Palmgren (:mats) 2007-04-11 18:55:54 PDT
(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...
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2007-04-11 19:06:21 PDT
> I was suspecting the code that creates first-letter frames

Hmm....  those can indeed go away and reappear as needed, you're right.
Comment 12 Aaron Leventhal 2007-04-11 19:27:01 PDT
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.
Comment 13 Aaron Leventhal 2007-04-11 19:29:28 PDT
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.
Comment 14 Aaron Leventhal 2007-04-11 19:31:25 PDT
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.
Comment 15 Aaron Leventhal 2007-04-11 19:56:43 PDT
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.
Comment 16 Aaron Leventhal 2007-04-11 20:02:50 PDT
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.
Comment 17 Aaron Leventhal 2007-04-11 20:04:04 PDT
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?
Comment 18 Mats Palmgren (:mats) 2007-04-11 21:05:13 PDT
(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 19 Mats Palmgren (:mats) 2007-04-11 21:07:16 PDT
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.
Comment 20 Lukas Loehrer 2007-04-11 23:51:56 PDT
> 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.
Comment 21 Lukas Loehrer 2007-04-12 01:48:26 PDT
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.

Comment 22 Mats Palmgren (:mats) 2007-04-12 07:49:39 PDT
Created attachment 261377 [details]
Standalone testcase for trunk, CRASH on load
Comment 23 Aaron Leventhal 2007-04-12 08:10:43 PDT
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.
Comment 24 Aaron Leventhal 2007-04-12 09:03:08 PDT
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?
Comment 25 Aaron Leventhal 2007-04-12 09:45:39 PDT
*** Bug 376884 has been marked as a duplicate of this bug. ***
Comment 26 Daniel Veditz [:dveditz] 2007-04-16 19:17:26 PDT
Mats's testcase in comment 22 crashes FF2.0.0.4pre. Nominating for the branch.
Comment 27 Daniel Veditz [:dveditz] 2007-04-18 16:01:15 PDT
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.
Comment 28 Aaron Leventhal 2007-05-15 10:25:46 PDT
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?
Comment 29 Mats Palmgren (:mats) 2007-05-16 18:12:12 PDT
Created attachment 265073 [details] [diff] [review]
Patch for branches
Comment 30 Mats Palmgren (:mats) 2007-05-16 18:16:25 PDT
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.
Comment 31 Aaron Leventhal 2007-05-16 19:05:19 PDT
Mats, how should we deal with the fact that it still crashes?
Comment 32 Mats Palmgren (:mats) 2007-05-16 19:22:40 PDT
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...
Comment 33 Aaron Leventhal 2007-05-16 19:28:38 PDT
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.
Comment 34 Mats Palmgren (:mats) 2007-05-16 20:51:43 PDT
Spawned off bug 380975 for the additional branch null-ptr crash.
Comment 35 Daniel Veditz [:dveditz] 2007-06-22 11:13:57 PDT
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
Comment 36 Mats Palmgren (:mats) 2007-07-01 16:08:36 PDT
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.
Comment 37 Carsten Book [:Tomcat] 2007-07-05 05:51:14 PDT
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
Comment 38 Carsten Book [:Tomcat] 2007-08-23 08:07:49 PDT
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

Note You need to log in before you can comment on or make changes to this bug.