377767 - Crash when closing Page Info dialog at the Media tab.

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[[:Aleksej]]

Minefield 2007041704, Debian Etch.
TB31271046E, TB31271212Y, TB31271351M, TB31271046E

Also experienced bug 377123 / bug 377732 on this system.

Comment 1

[[:Aleksej]]

Incident ID: 31271046
Stack Signature	 0x00000029 640df0ac
Product ID	FirefoxTrunk
Build ID	2007041704
Trigger Time	2007-04-17 05:16:51.0
Platform	LinuxIntel
Operating System	Linux 2.6.18-4-k7
Module	
URL visited	
User Comments	Alt-F4 in Page Info likely bug 377123 / bug 377123 that is supposed to have been fixed in this build :(
Since Last Crash	0 sec
Total Uptime	0 sec
Trigger Reason	SIGSEGV: Segmentation Fault: (signal 11)
Source File, Line No.	N/A
Stack Trace 	
0x00000029
nsQueryInterface::operator()  [mozilla/xpcom/build/nsCOMPtr.cpp, line 47]
nsCOMPtr_base::assign_from_qi()  [mozilla/xpcom/build/nsCOMPtr.cpp, line 96]
nsAccessible::Shutdown()  [mozilla/accessible/src/base/nsAccessible.cpp, line 897]
nsAccessNode::ClearCacheEntry()  [mozilla/accessible/src/base/nsAccessNode.cpp, line 693]
nsBaseHashtable<nsVoidHashKey, nsCOMPtr<nsIAccessNode>, nsIAccessNode*>::s_EnumStub()
PL_DHashTableEnumerate()  [mozilla/xpcom/build/pldhash.c, line 724]
nsAccessNode::ClearCache()  [mozilla/accessible/src/base/nsAccessNode.cpp, line 218]
nsDocAccessible::Shutdown()  [mozilla/accessible/src/base/nsDocAccessible.cpp, line 712]
nsDocAccessible::Destroy()  [mozilla/accessible/src/base/nsDocAccessible.cpp, line 502]
nsRootAccessible::HandleEventWithTarget()  [mozilla/accessible/src/base/nsRootAccessible.cpp, line 846]
nsRootAccessibleWrap::HandleEventWithTarget()  [mozilla/accessible/src/atk/nsRootAccessibleWrap.cpp, line 62]
nsRootAccessible::HandleEvent()  [mozilla/accessible/src/base/nsRootAccessible.cpp, line 840]
nsEventListenerManager::HandleEventSubType()  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1093]
nsEventListenerManager::HandleEvent()  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1055]
nsEventTargetChainItem::HandleEvent()  [mozilla/content/events/src/nsEventDispatcher.cpp, line 1038]
nsEventTargetChainItem::HandleEventTargetChain()  [mozilla/content/events/src/nsEventDispatcher.cpp, line 238]
nsEventDispatcher::Dispatch()  [mozilla/content/events/src/nsEventDispatcher.cpp, line 477]
nsDocument::DispatchEventToWindow()  [mozilla/content/base/src/nsDocument.cpp, line 5649]
nsDocument::OnPageHide()  [mozilla/content/base/src/nsDocument.cpp, line 5704]
DocumentViewerImpl::PageHide()  [mozilla/layout/base/nsDocumentViewer.cpp, line 1237]
nsDocShell::FirePageHideNotification()  [mozilla/docshell/base/nsDocShell.cpp, line 62]
nsDocShell::Destroy()  [mozilla/docshell/base/nsDocShell.cpp, line 1055]
nsXULWindow::Destroy()  [mozilla/xpfe/appshell/src/nsXULWindow.cpp, line 712]
nsWebShellWindow::Destroy()  [mozilla/xpfe/appshell/src/nsWebShellWindow.cpp, line 814]
nsWebShellWindow::HandleEvent()  [mozilla/xpfe/appshell/src/nsWebShellWindow.cpp, line 371]
nsCommonWidget::DispatchEvent()  [mozilla/widget/src/gtk2/nsCommonWidget.cpp, line 220]
nsWindow::OnDeleteEvent()  [mozilla/widget/src/gtk2/nsWindow.cpp, line 936]
delete_event_cb()  [mozilla/widget/src/gtk2/nsWindow.cpp, line 4325]
libgtk-x11-2.0.so.0 + 0x12e250 (0xb7baf250)
libgobject-2.0.so.0 + 0x998b (0xb792498b)
libgobject-2.0.so.0 + 0x19f2d (0xb7934f2d)
libgobject-2.0.so.0 + 0x1b208 (0xb7936208)
libgobject-2.0.so.0 + 0x1b5d9 (0xb79365d9)
libgtk-x11-2.0.so.0 + 0x217f64 (0xb7c98f64)
libgtk-x11-2.0.so.0 + 0x128ebf (0xb7ba9ebf)
libgdk-x11-2.0.so.0 + 0x42eea (0xb7a28eea)
libglib-2.0.so.0 + 0x2b731 (0xb78b3731)
libglib-2.0.so.0 + 0x2e7a6 (0xb78b67a6)
libglib-2.0.so.0 + 0x2ed27 (0xb78b6d27)
nsBaseAppShell::DoProcessNextNativeEvent()  [mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp, line 138]
nsBaseAppShell::OnProcessNextEvent()  [mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp, line 247]
nsThread::ProcessNextEvent()  [mozilla/xpcom/threads/nsThread.cpp, line 846]
NS_ProcessNextEvent_P()  [mozilla/xpcom/build/nsThreadUtils.cpp, line 227]
nsBaseAppShell::Run()  [mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp, line 154]
nsAppStartup::Run()  [mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 172]
XRE_main()  [mozilla/toolkit/xre/nsAppRunner.cpp, line 2891]
main()  [mozilla/browser/app/nsBrowserApp.cpp, line 62]
libc.so.6 + 0x14ea8 (0xb738fea8)

Comment 2

[Aaron Leventhal]

This is reproduceable on Linux, but not on Windows.

Comment 3

[Aaron Leventhal]

Ginn, do you have time to look at this?

Comment 4

[Ginn Chen]

It's not 100% reproducible, around 50% on my PC.

I don't know if it's related, but
I didn't reproduce it if I back out this change of bug 372367.
RCS file: /cvsroot/mozilla/accessible/src/xul/nsXULFormControlAccessible.cpp,v

-NS_IMPL_ISUPPORTS_INHERITED2(nsXULTextFieldAccessible, nsAccessible, nsIAccessibleText, nsIAccessibleEditableText)
+NS_IMPL_ISUPPORTS_INHERITED1(nsXULTextFieldAccessible, nsHyperTextAccessible,
+                             nsIAccessibleText)

surkov, can you take a look?

Comment 5

[alexander :surkov (:asurkov)]

Not sure how soon I can because I'm new a bit in linux and I didn't setup it for the work completely yet.

Comment 6

[Mats Palmgren (inactive)]

The root cause of the problem is that we run Shutdown() from
nsAccessNode::~nsAccessNode() (bug 377737).
In this particular case we want to run nsAccessible::Shutdown()
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/base/nsAccessible.cpp&rev=1.256&root=/cvsroot&mark=562#545

but since we don't invoke that method our parent will have a mFirstChild which
points to a destroyed node.

Comment 7

[Aaron Leventhal]

Thanks Mats, did you see my question in that bug?

Comment 8

[Mats Palmgren (inactive)]

This is unrelated to bug 377737.
The real issue is that we have a nsHTMLTextFieldAccessible and a 
nsXULTextFieldAccessible that have the same 'mFirstChild' pointer.
We will crash in Shutdown() for the node that did SetFirstChild() first
since the last one "steals" the child (the child corresponds to the
anonymous DIV inside the text control).

Comment 9

[Mats Palmgren (inactive)]

Attachment: wip

This wallpapers over the problem, I'm not sure what the right fix is...

Comment 10

[Evan Yan]

In case it may help, bug 366500 is another bug about nsXULTextFieldAccessible and nsHTMLTextFieldAccessible sharing the same mFirstChild

Comment 11

[alexander :surkov (:asurkov)]

Mats, the problem is temporary accessible for html input, right?

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #11)
> Mats, the problem is temporary accessible for html input, right?

I guess you mean this one:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/xul/nsXULFormControlAccessible.cpp&rev=1.79&root=/cvsroot&mark=744#730

Good guess, but no cigar, that's not the one...

Comment 13

[Mats Palmgren (inactive)]

Attachment: Stack traces

Here's a few stack traces, the first two are when the accessible
objects involved are created, the third trace is when SetFirstChild
steals the child node, and the last trace is the crash.
I've highlighted the accessible node addresses.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

It seems the root cause for creating the nsHTMLTextFieldAccessible is
the InvalidateSubtreeFor() call from nsFrameManager::ReResolveStyleContext.
Using the binding parent (if any) fixes the crash (and no assertion occurs).

I've left the wallpaper in SetFirstChild() for now, I'm not sure if you want
that, but we should at least leave the assertion (wrapped in NS_DEBUG),
if not the actual InvalidateChildren() call...

Comment 15

[Aaron Leventhal]

So the HTML Input is setting its anonymous text as its first child, and that was already the XUL texbox's first child? So the HTML input is stealing it from the XUL textbox?

Wouldn't that happen any time the HTML input is asked for a child? That would cause it to CacheChildren().

I'd prefer not to add code to layout. If we need to grab the binding parent we could be doing that inside nsAccessibilityService::InvalidateSubtreeFor(). In fact, that method might want to use nsAccessibilityService::GetRelevantContentNodeFor(), which will make sure we start from the right content node.

However, that seems to fix only one situation where CacheChildren() occurs on the anon HTML input accessible.

Comment 16

[Aaron Leventhal]

Attachment: Slightly better

Mats, if you don't mind me writing the next version of this patch -- thanks a lot for figuring it out.

Comment 17

[Aaron Leventhal]

Comment on attachment 262180 [details] [diff] [review]
Patch rev. 1

I favor a slightly modified version of this, please see the next patch.

Comment 18

[alexander :surkov (:asurkov)]

Aaron, do we have a case when attached accessible doesn't allow accessible inside of anonymous content but it sets first child accessible from anonymous content. Is it valid case?

Comment 19

[Aaron Leventhal]

No that doesn't seem valid.

Comment 20

[alexander :surkov (:asurkov)]

Aaron, should we check if node is relevant when we fire ally event in FireAccessible(Toolkit)Event and if this node is not relevant then to do not fire ally event. I just look at AttributeChanged code and I do not get why do we fire event for attached accessible if attribute was changed on its child that isn't allowed to be accessible.

Comment 21

[Aaron Leventhal]

I wasn't sure if we should fire event if accessible is not attached.

I'll change it just to not fire event if node is not relevant.

Comment 22

[Aaron Leventhal]

Attachment: Just don't fire events for non-relevant accessibles

Comment 23

[Aaron Leventhal]

In the patch, it should be
s/IsRelevantNode/IsNodeRelevant

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 262656 [details] [diff] [review]
Just don't fire events for non-relevant accessibles

r=me

> 
>+PRBool nsAccessible::IsNodeRelevant(nsIDOMNode *aNode)
>+{
>+  // Don't fire events for accessibles that aren't an attached descendant of the document

Comment isn't clear. Here there is not any events. You may want to say this method is used when event is fired or something. Also do we want assertion because it looks it is not normal when this method return false.

>+  nsCOMPtr<nsIAccessibilityService> accService =
>+    do_GetService("@mozilla.org/accessibilityService;1");

Please use GetAccService()

Comment 25

[Aaron Leventhal]

GetAccService() wasn't available for static method.

Comment 26

[[:Aleksej]]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a4pre) Gecko/20070425 Minefield/3.0a4pre