Last Comment Bug 506206 - ASSERTION: There should always be a DOM node for an event: 'Not Reached', accessible/src/base/AccEvent.cpp, line 188
: ASSERTION: There should always be a DOM node for an event: 'Not Reached', acc...
Status: RESOLVED FIXED
: access, regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
Depends on: 420845
Blocks: 444067
  Show dependency treegraph
 
Reported: 2009-07-24 01:41 PDT by alexander :surkov
Modified: 2011-10-11 16:31 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.59 KB, patch)
2009-07-24 06:24 PDT, alexander :surkov
mzehe: review+
Details | Diff | Splinter Review
patch (15.37 KB, patch)
2009-07-24 06:54 PDT, alexander :surkov
ginn.chen: review+
Details | Diff | Splinter Review
patch (next iteration) (1.80 KB, patch)
2009-07-30 22:24 PDT, alexander :surkov
ginn.chen: review+
Details | Diff | Splinter Review
patch2 (next iteration) (1.75 KB, patch)
2009-08-03 23:35 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch3 (next iteration) (1.76 KB, patch)
2009-08-03 23:39 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch4 (next iteration) (10.76 KB, patch)
2009-08-04 01:39 PDT, alexander :surkov
ginn.chen: review+
Details | Diff | Splinter Review
patch5 (next iteration) (238.09 KB, patch)
2009-08-04 20:35 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch6 (ni) (10.79 KB, patch)
2009-08-04 21:15 PDT, alexander :surkov
neil: superreview+
Details | Diff | Splinter Review
one more patch (3.42 KB, patch)
2011-10-07 04:47 PDT, alexander :surkov
dbolter: review+
Details | Diff | Splinter Review

Description alexander :surkov 2009-07-24 01:41:17 PDT
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 2009-07-24 06:24:44 PDT
Created attachment 390448 [details] [diff] [review]
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) on PTO until August 15 2009-07-24 06:39:44 PDT
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 2009-07-24 06:54:01 PDT
Created attachment 390452 [details] [diff] [review]
patch
Comment 4 alexander :surkov 2009-07-27 18:31:05 PDT
landed on 1.9.2 http://hg.mozilla.org/mozilla-central/rev/5ead27dbc476
Comment 5 alexander :surkov 2009-07-28 02:07:54 PDT
reopen since assertion is still presented because application root accessible hasn't DOM node.
Comment 6 alexander :surkov 2009-07-30 22:24:09 PDT
Created attachment 391793 [details] [diff] [review]
patch (next iteration)

hack fix to get rid assertion
Comment 7 Ginn Chen 2009-08-02 23:52:47 PDT
Comment on attachment 391793 [details] [diff] [review]
patch (next iteration)

r=me

"duiring" typo
Comment 8 alexander :surkov 2009-08-03 21:24:04 PDT
landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/d6c10d88e4f6
Comment 9 alexander :surkov 2009-08-03 21:46:51 PDT
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 2009-08-03 21:47:14 PDT
I backed out this patch - http://hg.mozilla.org/mozilla-central/rev/86515f41793b
Comment 11 alexander :surkov 2009-08-03 23:35:57 PDT
Created attachment 392438 [details] [diff] [review]
patch2 (next iteration)
Comment 12 alexander :surkov 2009-08-03 23:39:03 PDT
Created attachment 392440 [details] [diff] [review]
patch3 (next iteration)
Comment 13 alexander :surkov 2009-08-04 00:37:18 PDT
Comment on attachment 392440 [details] [diff] [review]
patch3 (next iteration)

wrong patch again
Comment 14 alexander :surkov 2009-08-04 01:39:30 PDT
Created attachment 392449 [details] [diff] [review]
patch4 (next iteration)
Comment 15 neil@parkwaycc.co.uk 2009-08-04 05:32:49 PDT
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 neil@parkwaycc.co.uk 2009-08-04 05:48:39 PDT
Comment on attachment 392449 [details] [diff] [review]
patch4 (next iteration)

>+    nsRefPtr<nsApplicationAccessibleWrap> applicationAcc = 
Trailing space ;-)
Comment 17 alexander :surkov 2009-08-04 20:31:10 PDT
(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 2009-08-04 20:35:15 PDT
Created attachment 392654 [details] [diff] [review]
patch5 (next iteration)

with Neil's comments
Comment 19 alexander :surkov 2009-08-04 21:15:01 PDT
Created attachment 392658 [details] [diff] [review]
patch6 (ni)

patch 5 is from another bug, sorry for the spam
Comment 20 neil@parkwaycc.co.uk 2009-08-05 03:18:59 PDT
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 neil@parkwaycc.co.uk 2009-08-05 03:21:39 PDT
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 neil@parkwaycc.co.uk 2009-08-05 04:35:18 PDT
Weird. Even with the patch, I got an assertion ostensibly for a mouse move.
Comment 23 alexander :surkov 2009-08-21 04:35:40 PDT
part2 patch is landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/2826f35aa9ea, leaving the bug open.
Comment 24 alexander :surkov 2011-03-10 03:48:00 PST
(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 2011-10-07 04:47:21 PDT
Created attachment 565493 [details] [diff] [review]
one more patch
Comment 26 David Bolter [:davidb] 2011-10-07 07:01:00 PDT
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 2011-10-07 07:28:27 PDT
(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) 2011-10-07 07:51:18 PDT
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 2011-10-07 07:53:44 PDT
(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] 2011-10-07 10:11:22 PDT
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 2011-10-07 10:13:33 PDT
(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] 2011-10-07 11:22:32 PDT
(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 2011-10-11 08:34:46 PDT
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3764df1a70
Comment 34 Ed Morley [:emorley] 2011-10-11 16:31:02 PDT
https://hg.mozilla.org/mozilla-central/rev/bb3764df1a70

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