Martijn Wargers (dead) Reporter
	Description • 17 years ago

See testcase, you need to download the testcase to your computer to see the crash, because of enhanced privileges.

This doesn't seem to crash on branch, so might be a regression. I can look for a regression range, if wanted.

http://crash-stats.mozilla.com/report/index/1a252fd9-565a-11dc-9d33-001a4bd43ef6
0  	nsDocAccessible::TakeFocus()  	 mozilla/accessible/src/base/nsDocAccessible.cpp:286
1 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
2 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2285
3 	nsXPConnect::Release() 	mozilla/js/src/xpconnect/src/nsXPConnect.cpp:58
4 	XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*) 	mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1479
5 	XPCWrappedNative::GetWrappedNativeOfJSObject(JSContext*, JSObject*, JSObject*, JSObject**, XPCWrappedNativeTearOff**) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:1307

	Evan Yan Assignee
	Comment 1 • 17 years ago

null presShell got from doc accessible (iframe), because the iframe was set display=none.

	Aaron Leventhal
	Comment 2 • 17 years ago

Example of a document that is not accesssible -- it is on a display:none iframe.

	Aaron Leventhal
	Comment 3 • 17 years ago

Comment on attachment 278935 [details] [diff] [review]
null check

Is the iframe made display:none dynamically? Or was it display:none all the time?

I'm wondering if the nsDocAccessible should exist or not for a display:none iframe, but it's probably safer if it does.

Does it at least return the correct nsIAccessibleStates::STATE_INVISIBLE?

	Evan Yan Assignee
	Comment 4 • 17 years ago

(In reply to comment #3)
> (From update of attachment 278935 [details] [diff] [review])
> Is the iframe made display:none dynamically? Or was it display:none all the
> time?
> 
Yes, it was made dynamically in javascript, you can see it in the testcase.

> I'm wondering if the nsDocAccessible should exist or not for a display:none
> iframe, but it's probably safer if it does.
> 
It was loaded at the first time, then made display=none, so I think the doc accessible should exist.

> Does it at least return the correct nsIAccessibleStates::STATE_INVISIBLE?
> 
It doesn't on my testing.
But I think it's hard to tell, because the doc accessible is just an instant accessible, and its presShell goes null, itself goes to be shutdown'ed. The result could be random, depending on what time we hit GetState().

	Aaron Leventhal
	Comment 5 • 17 years ago

Ah, then it should at least be DEFUNCT, which is all it needs.

Basically this is just another case of an nsIAccessible* method that needs to check whether the accessible node is shutdown, before executing any other code.

We often check that with mDOMNode, but checking pres shell works too.

	Evan Yan Assignee
	Comment 6 • 17 years ago

mDOMNode is still validate when we go into nsDocAccessible::TakeFocus(), it has even been used:

 281   nsCOMPtr<nsIDocShellTreeItem> treeItem = GetDocShellTreeItemFor(mDOMNode);

But presShell has already been null, then in GetPresSell(), the accessible got shutdown'ed.

	Aaron Leventhal
	Comment 7 • 17 years ago

> itself goes to be shutdown'ed

I thought all shut down accessibles have mDOMNode = nsnull

Something isn't right.

	Evan Yan Assignee
	Comment 8 • 17 years ago

Sorry, I should have made it more clear. The sequence is like below.

in javascript call accessible.takeFocus()
=>nsDocAccessible::TakeFocus()
  mDOMNode is valid for now;
  call GetState();
  call GetPresShell();
  =>nsAccessNode::GetPresShell()
    found presShell is null, call Shutdown();
    =>nsAccessNode::Shutdown()
      mDOMNode is set to null now.

I'm not sure when presShell goes null, the setting of display=none and the calling to takeFocus() are asynchronous.

	Aaron Leventhal
	Comment 9 • 17 years ago

The shutdown should happen in a more deliberate way, but us being notified about the iframe going away, not by us noticing that there's no longer a presshell.

If you don't have time to figure out what notification isn't happening or isn't working correctly, then we should get this fix in but leave it open for a proper fix.

	Evan Yan Assignee
	Comment 10 • 17 years ago

How us were notified? by some dom event?

Could it be possible that the notification was still on the way when we run into this crash? When I was stepping in the function in gdb, the crash could not be reproducible. I'll dig into it tomorrow.

	Aaron Leventhal
	Comment 11 • 17 years ago

Put a breakpoint in nsDocAccessible::InvalidateCacheSubtree()

You might want to add some temporary lines to it like:

#ifdef DEBUG_A11Y
  if (aChangeNode == mDOMNode) {
    printf("\nDoc invalidation\n");   // Set breakpoint here
  }
#endif

	Evan Yan Assignee
	Comment 12 • 17 years ago

Usually, doc accessible was shutdown'ed when we received "pagehide" event.
When the iframe was set invisible by javascript, we didn't get any dom event (should we?), the doc accessible for the iframe didn't get shutdown'ed until somewhere we noticed the presShell is null. After the doc accessible for the iframe get Shutdown'ed, it will have STATE_DEFUNCT.

So I think the patch is the right fix.

	Aaron Leventhal
	Comment 13 • 17 years ago

Yes, we should get a notification from layout. Search for InvalidateSubtreeFor in layout. That's how we invalidate for example a link accessible when it is made display: none

	Aaron Leventhal
	Comment 14 • 17 years ago

IOW this patch is a temporary fix but we should still put it in.

	Aaron Leventhal
	Comment 15 • 17 years ago

Checked in for Evan, but I'm leaving this open so we can eventually find a way to shut down the doc accessible properly via notifications from layout.

	Martijn Wargers (dead) Reporter
	Comment 16 • 17 years ago

I have verified that the testcase doesn't crash anymore, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre

	Evan Yan Assignee
	Comment 17 • 17 years ago

(In reply to comment #13)
> Yes, we should get a notification from layout. Search for InvalidateSubtreeFor
> in layout. That's how we invalidate for example a link accessible when it is
> made display: none
> 

Aaron, it seems that InvalidateCacheSubtree() just fires some events but doesn't invalidate accessibles when something hide. Is that expected?

	Aaron Leventhal
	Comment 18 • 17 years ago

Evan, we fire it on a delay now for EVENT_ASYNCH_HIDE.

We do it here:
http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsDocAccessible.cpp#1627

Should we not be doing it on a delay?

	Evan Yan Assignee
	Comment 19 • 17 years ago

In the case of this bug, it's because we didn't fire EVENT_ASYNCH_HIDE.

The structure of dom nodes and accessible nodes:

HTMLDocument          --> doc accessible
  \--HTMLBodyElement   --> no corresponding accessible

When document.body.style.display = 'none', InvalidateSubtreeFor is called on HTMLBodyElement. Because we didn't get corresponding accessible for the dom node, EVENT_ASYNCH_HIDE is not fired.

	Evan Yan Assignee
	Comment 21 • 17 years ago

Comment on attachment 282699 [details] [diff] [review]
GetAccessibleInParentChain when didn't get accessible and !isShowing

After reading the comment of https://bugzilla.mozilla.org/show_bug.cgi?id=393660#c2

I found the patch is not correct.

I think we can close this bug, since the crash is already fixed in the former patch, and remaining issue seems dupe of bug 393660.

	Aaron Leventhal
	Comment 22 • 17 years ago

I don't think it's a dupe of bug 393660.

Evan, I need 2 things at least:
1) devise an assertion for the core issue
2) describe what is really happening

If you really want to close this bug, then we should have the assertion and a description of the problem -- that should probably be in a new bug.

We need to make sure the core issues are resolved.

	Evan Yan Assignee
	Comment 23 • 17 years ago

Aaron, what do you think about comment 19? That's the reason I think this bug is dupe of bug 393660.

	Aaron Leventhal
	Comment 24 • 17 years ago

Evan, you're right -- but it needs to be tested to be sure, right?

I tested it and it does prevent the |if (!shell) return;| from being called

	Aaron Leventhal
	Comment 25 • 17 years ago

Actually I still see a problem, because EVENT_ASYNCH_HIDE is not fired on the parent with the accessible, thus InvalidateChildren() doesn't happen.

We probably need to fire a special INVALIDATE_CHILDREN_INTERNAL event so that FlushPendingEvents() does the InvalidateChildren() at the right time.

	Aaron Leventhal
	Comment 26 • 17 years ago

Followup issue was already filed in bug 398205.

	Aaron Leventhal
	Comment 27 • 17 years ago

I can't duplicate the conditions of this bug for some reason, at the moment.