Closed Bug 506206 Opened 11 years ago Closed 8 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access, regression)

Attachments

(3 files, 6 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #390448 - Flags: review?(marco.zehe)
Attachment #390448 - Flags: review?(ginn.chen)
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.
Attachment #390448 - Flags: review?(marco.zehe) → review+
Attached patch patchSplinter Review
Attachment #390448 - Attachment is obsolete: true
Attachment #390452 - Flags: review?(ginn.chen)
Attachment #390448 - Flags: review?(ginn.chen)
Attachment #390452 - Flags: review?(ginn.chen) → review+
landed on 1.9.2 http://hg.mozilla.org/mozilla-central/rev/5ead27dbc476
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
reopen since assertion is still presented because application root accessible hasn't DOM node.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch (next iteration) (obsolete) — Splinter Review
hack fix to get rid assertion
Attachment #391793 - Flags: review?(ginn.chen)
Comment on attachment 391793 [details] [diff] [review]
patch (next iteration)

r=me

"duiring" typo
Attachment #391793 - Flags: review?(ginn.chen) → review+
landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/d6c10d88e4f6
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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’
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch patch2 (next iteration) (obsolete) — Splinter Review
Attachment #391793 - Attachment is obsolete: true
Attachment #392438 - Flags: superreview?(neil)
Attached patch patch3 (next iteration) (obsolete) — Splinter Review
Attachment #392438 - Attachment is obsolete: true
Attachment #392440 - Flags: superreview?(neil)
Attachment #392438 - Flags: superreview?(neil)
Comment on attachment 392440 [details] [diff] [review]
patch3 (next iteration)

wrong patch again
Attachment #392440 - Attachment is obsolete: true
Attachment #392440 - Flags: superreview?(neil)
Attached patch patch4 (next iteration) (obsolete) — Splinter Review
Attachment #392449 - Flags: superreview?(neil)
Attachment #392449 - Flags: review?(ginn.chen)
Attachment #392449 - Flags: review?(ginn.chen) → review+
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 on attachment 392449 [details] [diff] [review]
patch4 (next iteration)

>+    nsRefPtr<nsApplicationAccessibleWrap> applicationAcc = 
Trailing space ;-)
(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?
Attached patch patch5 (next iteration) (obsolete) — Splinter Review
with Neil's comments
Attachment #392449 - Attachment is obsolete: true
Attachment #392654 - Flags: superreview?(neil)
Attachment #392449 - Flags: superreview?(neil)
Attached patch patch6 (ni)Splinter Review
patch 5 is from another bug, sorry for the spam
Attachment #392654 - Attachment is obsolete: true
Attachment #392658 - Flags: superreview?(neil)
Attachment #392654 - Flags: superreview?(neil)
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 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.
Attachment #392658 - Flags: superreview?(neil) → superreview+
Weird. Even with the patch, I got an assertion ostensibly for a mouse move.
part2 patch is landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/2826f35aa9ea, leaving the bug open.
(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?
Summary: 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 → ASSERTION: There should always be a DOM node for an event: 'Not Reached', file c:/mozilla/fx07-22/accessible/src/base/nsAccEvent.cpp, line 188
Summary: ASSERTION: There should always be a DOM node for an event: 'Not Reached', file c:/mozilla/fx07-22/accessible/src/base/nsAccEvent.cpp, line 188 → ASSERTION: There should always be a DOM node for an event: 'Not Reached', file c:/mozilla/fx07-22/accessible/src/base/AccEvent.cpp, line 188
Summary: ASSERTION: There should always be a DOM node for an event: 'Not Reached', file c:/mozilla/fx07-22/accessible/src/base/AccEvent.cpp, line 188 → ASSERTION: There should always be a DOM node for an event: 'Not Reached', accessible/src/base/AccEvent.cpp, line 188
Attached patch one more patchSplinter Review
Attachment #565493 - Flags: review?(bolterbugz)
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.
(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 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.
(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 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.
Attachment #565493 - Flags: review?(bolterbugz) → review+
(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?
(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.
https://hg.mozilla.org/mozilla-central/rev/bb3764df1a70
Status: REOPENED → RESOLVED
Closed: 11 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.