506206 - ASSERTION: There should always be a DOM node for an event: 'Not Reached', accessible/src/base/AccEvent.cpp, line 188

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Getting an assertion when opening new window

ASSERTION: There should always be a DOM node for an event: 'Not Reached', file c:/mozilla/fx07-22/accessible/src/base/nsAccessibleEventData.cpp, line 114

the problem we trying to fire reorder event for mParent when doc accessible is created but mParent is not initialized for root accessible. Either we should initialize mParent or use GetParent. Assertion appeared starting from bug 420845.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

1) Make nsRootAccessible::GetParent to cache parent like it happens in nsDocAccessible
2) Call nsDocAcc::Init() after we append root acc to app root acc to fire reorder event when we constructed the tree

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 390448 [details] [diff] [review]
patch

r=me with a nit:

>+      var expextedFirstChild = childCount > 0 ?
>+        children.queryElementAt(0, nsIAccessible) : null;

>+      is(firstChild, expextedFirstChild,
+         "Wrong first child of " + prettyName(acc));

These should be "expectedFirstChild", replace the second x with a c please.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[alexander :surkov (:asurkov)]

landed on 1.9.2 http://hg.mozilla.org/mozilla-central/rev/5ead27dbc476

Comment 5

[alexander :surkov (:asurkov)]

reopen since assertion is still presented because application root accessible hasn't DOM node.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch (next iteration)

hack fix to get rid assertion

Comment 7

[Ginn Chen]

Comment on attachment 391793 [details] [diff] [review]
patch (next iteration)

r=me

"duiring" typo

Comment 8

[alexander :surkov (:asurkov)]

landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/d6c10d88e4f6

Comment 9

[alexander :surkov (:asurkov)]

not compiled on linux:

/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessibleEventData.cpp: In member function ‘void nsAccEvent::CaptureIsFromUserInput(PRBool)’:
/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessibleEventData.cpp:120: error: conversion from ‘nsRefPtr<nsApplicationAccessibleWrap>’ to non-scalar type ‘nsCOMPtr<nsIAccessible>’ requested
/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessibleEventData.cpp: In constructor ‘nsAccTextChangeEvent::nsAccTextChangeEvent(nsIAccessible*, PRInt32, PRUint32, PRBool, PRBool)’:
/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessibleEventData.cpp:568: warning: enumeral mismatch in conditional expression: ‘nsIAccessibleEvent::<anonymous enum>’ vs ‘nsIAccessibleEvent::<anonymous enum>’
../../../dist/include/nsAutoPtr.h: In destructor ‘nsRefPtr<T>::~nsRefPtr() [with T = nsApplicationAccessibleWrap]’:
/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessibleEventData.cpp:119:   instantiated from here
../../../dist/include/nsAutoPtr.h:956: error: invalid use of incomplete type ‘struct nsApplicationAccessibleWrap’
/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessNode.h:68: error: forward declaration of ‘struct nsApplicationAccessibleWrap’
../../../dist/include/nsAutoPtr.h: In copy constructor ‘nsRefPtr<T>::nsRefPtr(const nsRefPtr<T>&) [with T = nsApplicationAccessibleWrap]’:
/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessibleEventData.cpp:119:   instantiated from here
../../../dist/include/nsAutoPtr.h:972: error: invalid use of incomplete type ‘struct nsApplicationAccessibleWrap’
/builds/moz2_slave/mozilla-central-linux64/build/accessible/src/base/nsAccessNode.h:68: error: forward declaration of ‘struct nsApplicationAccessibleWrap’

Comment 10

[alexander :surkov (:asurkov)]

I backed out this patch - http://hg.mozilla.org/mozilla-central/rev/86515f41793b

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2 (next iteration)

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3 (next iteration)

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 392440 [details] [diff] [review]
patch3 (next iteration)

wrong patch again

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch4 (next iteration)

Comment 15

[[e-mail]]

Comment on attachment 392449 [details] [diff] [review]
patch4 (next iteration)

>+    // XXX: remove this hack during reorganization of 506907. Meanwhile we
>+    // want to get rid an assertion for application accessible events which
>+    // don't have DOM node (see bug 506206).
>+    nsRefPtr<nsApplicationAccessibleWrap> applicationAcc = 
>+      nsAccessNode::GetApplicationAccessible();
>+
>+    if (mAccessible != static_cast<nsIAccessible*>(applicationAcc.get()))
>+      NS_NOTREACHED("There should always be a DOM node for an event");
I still get this assertion even with the patch. In my case, mAccessible is null; this is presumably because a top-level chrome document has no parent.

Anyway this should all be in an #ifdef DEBUG block to avoid calling GetApplicationAccessible without using it.

Also if (!foo) NS_NOTREACHED("no foo"); should be NS_ASSERTION(foo, "no foo");

Comment 16

[[e-mail]]

Comment on attachment 392449 [details] [diff] [review]
patch4 (next iteration)

>+    nsRefPtr<nsApplicationAccessibleWrap> applicationAcc = 
Trailing space ;-)

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> >+    if (mAccessible != static_cast<nsIAccessible*>(applicationAcc.get()))
> >+      NS_NOTREACHED("There should always be a DOM node for an event");
> I still get this assertion even with the patch. In my case, mAccessible is
> null; this is presumably because a top-level chrome document has no parent.

I can get it when I investigate Error Console by DOMi, when I expand unaccessible iframe (because it is collapsed) which has accessible document (it shouldn't be accessible I think). I would like rather to deal with it in another bug since it's bit a different issue. If you like I can address this peculiarity in bug summary. Does it work for you?

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch5 (next iteration)

with Neil's comments

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch6 (ni)

patch 5 is from another bug, sorry for the spam

Comment 20

[[e-mail]]

Well, this patch certainly improves things, although I still get the assertion whenever I open DOM Inspector more than once (the first time I guess it loads accessibility after the point at which the assertion would be triggered.)

>accessibility!nsAccEvent::CaptureIsFromUserInput(int aIsAsynch = 0)+0xc1 [mozilla\accessible\src\base\nsaccessibleeventdata.cpp @ 123]
>accessibility!nsAccEvent::nsAccEvent(unsigned int aEventType = 0x5c, class nsIAccessible * aAccessible = 0x00000000, int aIsAsynch = 0, nsAccEvent::EEventRule aEventRule = eCoalesceFromSameSubtree (1))+0x71 [mozilla\accessible\src\base\nsaccessibleeventdata.cpp @ 86]
>accessibility!nsAccReorderEvent::nsAccReorderEvent(class nsIAccessible * aAccTarget = 0x00000000, int aIsAsynch = 0, int aIsUnconditional = 1, class nsIDOMNode * aReasonNode = 0x0732aedc)+0x1b [mozilla\accessible\src\base\nsaccessibleeventdata.cpp @ 476]
>accessibility!nsDocAccessible::Init(void)+0x122 [mozilla\accessible\src\base\nsdocaccessible.cpp @ 626]
>accessibility!nsAccessibilityService::CreateRootAccessible(class nsIPresShell * aShell = 0x08a24220, class nsIDocument * aDocument = 0x0732ae40, class nsIAccessible ** aRootAcc = 0x0012f734)+0x651 [mozilla\accessible\src\base\nsaccessibilityservice.cpp @ 482]
>accessibility!nsAccessibilityService::GetAccessible(class nsIDOMNode * aNode = 0x0732aedc, class nsIPresShell * aPresShell = 0x08a24220, class nsIWeakReference * aWeakShell = 0x08a24dc8, class nsIFrame ** aFrameHint = 0x0012f770, int * aIsHidden = 0x0012f778, class nsIAccessible ** aAccessible = 0x0012f7f0)+0x2fa [mozilla\accessible\src\base\nsaccessibilityservice.cpp @ 1394]
>accessibility!nsAccessibilityService::GetAccessibleInShell(class nsIDOMNode * aNode = 0x0732aedc, class nsIPresShell * aPresShell = 0x08a24220, class nsIAccessible ** aAccessible = 0x0012f7f0)+0xf1 [mozilla\accessible\src\base\nsaccessibilityservice.cpp @ 1257]
>accessibility!nsAccessibilityService::GetAccessibleFor(class nsIDOMNode * aNode = 0x0732aedc, class nsIAccessible ** aAccessible = 0x0012f7f0)+0x12f [mozilla\accessible\src\base\nsaccessibilityservice.cpp @ 1201]
>accessibility!nsAccEvent::GetAccessibleByNode(void)+0xc4 [mozilla\accessible\src\base\nsaccessibleeventdata.cpp @ 288]
>accessibility!nsAccEvent::GetAccessible(class nsIAccessible ** aAccessible = 0x0012f958)+0x55 [mozilla\accessible\src\base\nsaccessibleeventdata.cpp @ 226]
>accessibility!nsDocAccessible::FlushPendingEvents(void)+0x16a [mozilla\accessible\src\base\nsdocaccessible.cpp @ 1651]
>accessibility!nsDocAccessible::FlushEventsCallback(class nsITimer * aTimer = 0x08289170, void * aClosure = 0x061765c8)+0x41 [mozilla\accessible\src\base\nsdocaccessible.cpp @ 1817]
>xpcom_core!nsTimerImpl::Fire(void)+0x28a [mozilla\xpcom\threads\nstimerimpl.cpp @ 427]
>xpcom_core!nsTimerEvent::Run(void)+0xa1 [mozilla\xpcom\threads\nstimerimpl.cpp @ 521]
>xpcom_core!nsThread::ProcessNextEvent(int mayWait = 1, int * result = 0x0012fa44)+0x1f3 [mozilla\xpcom\threads\nsthread.cpp @ 528]
>xpcom_core!NS_ProcessNextEvent_P(class nsIThread * thread = 0x00c9fb10, int mayWait = 1)+0x51 [c:\hg\mozilla\xpcom\build\nsthreadutils.cpp @ 230]
>gkwidget!nsBaseAppShell::Run(void)+0x5d [mozilla\widget\src\xpwidgets\nsbaseappshell.cpp @ 170]
>tkitcmps!nsAppStartup::Run(void)+0x69 [mozilla\toolkit\components\startup\src\nsappstartup.cpp @ 193]
>xul!XRE_main(int argc = 6, char ** argv = 0x00c9b870, struct nsXREAppData * aAppData = 0x00c9c640)+0x2a13 [mozilla\toolkit\xre\nsapprunner.cpp @ 3392]
>seamonkey!NS_internal_main(int argc = 6, char ** argv = 0x00c9b870)+0x111 [suite\app\nssuiteapp.cpp @ 103]
>seamonkey!wmain(int argc = 6, unsigned short ** argv = 0x00c99720)+0x101 [mozilla\toolkit\xre\nswindowswmain.cpp @ 108]
>seamonkey!wmainCRTStartup(void)+0x12c
kernel32!BaseProcessStart+0x23

Comment 21

[[e-mail]]

Comment on attachment 392658 [details] [diff] [review]
patch6 (ni)

>+      NS_ASSERTION(eventNode, "There should always be a DOM node for an event");
Not exactly what I meant, but better than before at least.

Comment 22

[[e-mail]]

Weird. Even with the patch, I got an assertion ostensibly for a mouse move.

Comment 23

[alexander :surkov (:asurkov)]

part2 patch is landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/2826f35aa9ea, leaving the bug open.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> Weird. Even with the patch, I got an assertion ostensibly for a mouse move.

Neil, I can't reproduce it on trunk. Can you?

Comment 25

[alexander :surkov (:asurkov)]

Attachment: one more patch

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 565493 [details] [diff] [review]
one more patch

Review of attachment 565493 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/nsDocAccessible.cpp
@@ -1734,5 @@
>  {
> -  nsAccessible* accessible = aEvent->GetAccessible();
> -  if (!accessible)
> -    return;
> -

I don't see why this is okay to remove.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #26)

> ::: accessible/src/base/nsDocAccessible.cpp
> @@ -1734,5 @@
> >  {
> > -  nsAccessible* accessible = aEvent->GetAccessible();
> > -  if (!accessible)
> > -    return;
> > -
> 
> I don't see why this is okay to remove.

because of check in NotificationController

Comment 28

[Trevor Saunders (:tbsaunde)]

Comment on attachment 565493 [details] [diff] [review]
one more patch


>+    nsRefPtr<nsHyperTextAccessible> hyperText =
>+      do_QueryObject(aEvent->GetAccessible());

why not use AsHyperText()?

>+      nsRefPtr<AccEvent> caretMoveEvent = new AccCaretMoveEvent(hyperText,
>+                                                                caretOffset);
>       if (!caretMoveEvent)
>         return;

you don't need that null check.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #28)
> Comment on attachment 565493 [details] [diff] [review] [diff] [details] [review]
> one more patch

fixed after our irc chat :) thanks for watching!

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 565493 [details] [diff] [review]
one more patch

>+      ShutdownChildrenInSubtree(aEvent->GetAccessible());

I think we should null check, probably in ShutdownChildrenInSubtree.

r=me with that and Trevor's comments addressed.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #30)
> Comment on attachment 565493 [details] [diff] [review] [diff] [details] [review]
> one more patch
> 
> >+      ShutdownChildrenInSubtree(aEvent->GetAccessible());
> 
> I think we should null check, probably in ShutdownChildrenInSubtree.
> 
> r=me with that and Trevor's comments addressed.

null check aEvent->GetAccessible()? Why?

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander surkov from comment #31)
> (In reply to David Bolter [:davidb] from comment #30)

null check aEvent->GetAccessible()? Why?

OK I see, we're ok.

Comment 33

[alexander :surkov (:asurkov)]

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3764df1a70

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb3764df1a70