630841 - Crash in nsHyperTextAccessible::GetText()

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mihai Sucan [:msucan]]

Firefox 4 very easily crashes when I use Panorama (just drag/resize groups).

System info:

Pentium(R) Dual-Core CPU E6500 @ 2.93GHz, Ubuntu 10.04 LTS (amd64), Gnome 2.30.2, Metacity, Nvidia drivers (195.36.24), Xorg 1.7.6 ... all coming from the official Ubuntu repos (up-to-date).

Gnome has accessibility features enabled. Environment variable: GTK_MODULES="canberra-gtk-module:gail:atk-bridge".

Firefox 4 nightlies do not crash, only Firefox 4 local builds do. The .mozconfig I have is:

. $topsrcdir/browser/config/mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/ff-obj
mk_add_options MOZ_MAKE_FLAGS="-j4 -s"
ac_add_options --enable-optimize
ac_add_options --enable-tests

If I disable the Gnome accessibility features, Firefox no longer crashes.

Stack trace:

#0  0x00007ffff6bff607 in nsHyperTextAccessible::GetText(int, int, nsAString_internal&) () from ./libxul.so
#1  0x00007ffff6c053f8 in getTextCB () from ./libxul.so
#2  0x00007fffea790cd4 in ?? () from /usr/lib/gtk-2.0/modules/libatk-bridge.so
#3  0x00007ffff30b3fb8 in ?? () from /usr/lib/libgobject-2.0.so.0
#4  0x00007ffff30b5a76 in g_signal_emit_valist () from /usr/lib/libgobject-2.0.so.0
#5  0x00007ffff30b5dc8 in g_signal_emit_by_name () from /usr/lib/libgobject-2.0.so.0
#6  0x00007ffff6c00241 in nsAccessibleWrap::FireAtkTextChangedEvent(AccEvent*, _AtkObject*) () from ./libxul.so
#7  0x00007ffff6c00f3c in nsAccessibleWrap::FirePlatformEvent(AccEvent*) () from ./libxul.so
#8  0x00007ffff6bea964 in nsEventShell::FireEvent(AccEvent*) () from ./libxul.so
#9  0x00007ffff6bd9b57 in nsDocAccessible::ProcessPendingEvent(AccEvent*) () from ./libxul.so
#10 0x00007ffff6bd0e78 in NotificationController::WillRefresh(mozilla::TimeStamp) () from ./libxul.so
#11 0x00007ffff6403034 in nsRefreshDriver::Notify(nsITimer*) () from ./libxul.so
#12 0x00007ffff6d4f656 in nsTimerImpl::Fire() () from ./libxul.so
#13 0x00007ffff6d4f711 in nsTimerEvent::Run() () from ./libxul.so
#14 0x00007ffff6d4c918 in nsThread::ProcessNextEvent(int, int*) () from ./libxul.so
#15 0x00007ffff6d18f81 in NS_ProcessNextEvent_P(nsIThread*, int) () from ./libxul.so
#16 0x00007ffff6c64b8a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) () from ./libxul.so
#17 0x00007ffff6d7dc56 in MessageLoop::Run() () from ./libxul.so
#18 0x00007ffff6baf6b7 in nsBaseAppShell::Run() () from ./libxul.so
#19 0x00007ffff6a668ce in nsAppStartup::Run() () from ./libxul.so
#20 0x00007ffff628e995 in XRE_main () from ./libxul.so
#21 0x0000000000401c59 in main ()

Comment 1

[alexander :surkov (:asurkov)]

regression from bug 630001, part3 (fix getText).

Comment 2

[Fernando Herrera]

Attachment: patch fixing the crash

508     nsAccessible* child = GetChildAt(startChildIdx);
509     child->AppendTextTo(aText, aStartOffset - childOffset,
510                         aEndOffset - aStartOffset);

child is NULL because GetChildAt returns an wrong result (that child has been removed, but mOffsets has not been updated).

Comment 3

[alexander :surkov (:asurkov)]

landed on 2.0 (fx4beta12) - http://hg.mozilla.org/mozilla-central/rev/6cb543f7d5b0

Comment 4

[alexander :surkov (:asurkov)]

fer, could you create mochitest for this please?

Comment 5

[Fernando Herrera]

Attachment: mochitest case

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 509148 [details] [diff] [review]
mochitest case


>+    function removeChild(aContainerID, aChildID, aInitialText, aFinalText)

please add a comment before the function what it's going to test like

/**
 * Text offsets must be updated when hypertext child is removed.
 */

>+    {
>+      this.containerNode = getNode(aContainerID);
>+      this.container = getAccessible(this.containerNode, nsIAccessibleText);
>+      this.childNode = getNode(aChildID);
>+      this.child = getAccessible(this.childNode, nsIAccessibleHyperText);

this.child isn't used.

>+      this.initialText = aInitialText;
>+      this.finalText = aFinalText;

well you don't need to store them as members, you could refer as aInitialText and aFinalText

>+
>+      // Call first to getText so text is cached

the text doesn't get cached by this call, but offsets.

>+      this.finalCheck = function removeChild_finalCheck()
>+      {
>+        is(this.container.getText(0, -1), this.finalText,
>+           "Wrong text after child removal");
>+        is(this.container.characterCount, this.finalText.length,
>+           "Wrong text after child removal");
>+        for(var i = 0; i < this.finalText.length; i++) {
>+          is(this.container.getText(i, i + 1), this.finalText[i],
>+             "Wrong text after child removal");
>+        }

this loop isn't necessary actually, up to you to save it or get rid

>+      }
>+
>+      this.getID = function updateText_getID()

removeChild_getID()

>+      {
>+        return "check text after removing chuld from '" + aContainerID + "'";

chuld -> child

looks good, thanks, r=me, please

Comment 7

[Fernando Herrera]

Attachment: updated test case

Comment 8

[Mike Hommey [:glandium]]

FYI, I got this crash with 4.0b11 build1 (which is not unexpected given when the patch landed).

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Created attachment 509152 [details] [diff] [review]
> updated test case

I put it into my landing queue. I'll land eventually if you don't need it asap. Thanks for doing this!

Comment 10

[alexander :surkov (:asurkov)]

mochitest is landed - http://hg.mozilla.org/mozilla-central/rev/02b5151b2a19

Comment 11

[Marco Zehe (:MarcoZ)]

Landed on the 4.0b11 relbranch: http://hg.mozilla.org/mozilla-central/rev/6d9ac157fc84
And Mochitest landed on the relbranch: http://hg.mozilla.org/mozilla-central/rev/bb8247382c3e

Comment 12

[Fernando Herrera]

Cannot reproduce it anymore with 4.0b11 build3

Comment 13

[Marco Zehe (:MarcoZ)]

Setting to Verified basec on comment #12.