When nsRootAccessible receives pagehide event, it shuts down the doc accessible. But it doesn't receive it when a tab is closing, it caused the crash of bug 468727. When a page is reloading, or the window is closed, the pagehide event is delivered to nsRootAccessible. I tried to debug it, and I found the pagehide event is not dispatched to nsWindowRoot when a tab is closing. I'm not sure if it is desired. But we need to get the doc accessible shut down, otherwise we may consume extra memories.

It is the same with Firefox 3.0. I hope someone familiar with ESM can explain the desired behavior. Meanwhile: Perhaps we can shutdown it when we have EVENT_DOM_DESTROY for the tab panel. Surkov, what do you think?

(In reply to comment #1) > Meanwhile: > Perhaps we can shutdown it when we have EVENT_DOM_DESTROY for the tab panel. > Surkov, what do you think? Ginn, sounds right. We should shutdown any underlying documents for hidden elements. It's very interesting why we don't do this. Though looking at nsDocument::Shutdown (http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsDocAccessible.cpp#624), it shutdowns child documents explicitly.

(In reply to comment #1) > Meanwhile: > Perhaps we can shutdown it when we have EVENT_DOM_DESTROY for the tab panel. That might work, or maybe we could make a direct call from nsDocument::OnPageHide?

I don't have a good way to get nsDocAccessible from nsXULTabpanelAccessible. I think it's not easy to do a direct call from nsDocument::OnPageHide(), either. I think it would work if we move the "pagehide" event handler from nsRootAccessible to nsDocAccessible.

Move "pagehide" handling to nsDocAccessible

Comment on attachment 367565 [details] [diff] [review] patch >+ >+// --------------- nsIDOMEventListener Methods (3) ------------------------ >+ >+NS_IMETHODIMP nsDocAccessible::HandleEvent(nsIDOMEvent* aEvent) >+{ >+ // Turn DOM events in accessibility events NIT: into >-class nsRootAccessible : public nsDocAccessibleWrap, >- public nsIDOMEventListener >+class nsRootAccessible : public nsDocAccessibleWrap We'll need a new NS_ROOTACCESSIBLE_IMPL_CID right?

Ginn, why does "pagehide" isn't propogated to root document? I think it was our idea to register event listeners on top document or window to handle events from all documents and windows. Does "pagehide" is special? Did it work previously?

(In reply to comment #6) > (From update of attachment 367565 [details] [diff] [review]) > >-class nsRootAccessible : public nsDocAccessibleWrap, > >- public nsIDOMEventListener > >+class nsRootAccessible : public nsDocAccessibleWrap > > We'll need a new NS_ROOTACCESSIBLE_IMPL_CID right? I think we don't. nsIDOMEventListener is still implemented (through nsDocAccessibleWrap).

(In reply to comment #7) > Ginn, why does "pagehide" isn't propogated to root document? I think it was our > idea to register event listeners on top document or window to handle events > from all documents and windows. Does "pagehide" is special? Did it work > previously? I guess when a tab is closed, the document is detached from the window, therefore it is not propagated to the root document. I checked with Firefox 1.5, it doesn't work either.

(In reply to comment #9) > (In reply to comment #7) > > Ginn, why does "pagehide" isn't propogated to root document? I think it was our > > idea to register event listeners on top document or window to handle events > > from all documents and windows. Does "pagehide" is special? Did it work > > previously? > > I guess when a tab is closed, the document is detached from the window, > therefore it is not propagated to the root document. > I checked with Firefox 1.5, it doesn't work either. Interesting. I'm okay with this patch, but we should probably ask layout folks if why they detach before sending the pagehide event (if that's what happens).

Maybe the problem is pagehide is untrasted?

It sounds untrusted events are not guilty. But I still don't understand why pagehide events for closing tab is special, because for example root accessible gets pagehide events from any closed child window.

Ok, I think Ginn is right and we don't get pagehide events because the node containing browser element is out of DOM. I'm not sure movement of pagehide event processing to document accessible is correct because we should start to get pagehide event for every subdocument and it can contradict we do in the meantime (i.e. shutdown any child document when parent document shutdown). In this light I think it might be worth to follow original Ginn's suggestion (see comment #1, comment #2).

Comment on attachment 367565 [details] [diff] [review] patch >+nsresult nsDocAccessible::HandleEventWithTarget(nsIDOMEvent* aEvent, >+ nsIDOMNode* aTargetNode) >+{ >+ nsAutoString eventType; >+ aEvent->GetType(eventType); >+ if (eventType.EqualsLiteral("pagehide")) { Ginn, can we rely on EVENT_DOM_DESTROY instead? (Your comment #1) I don't know the history here. >+ // pagehide event can be fired under several conditions, such as HTML >+ // document going away, closing a window/dialog, and wizard page changing. >+ // We only destroy the accessible object when it's a document accessible, >+ // so that we don't destroy something still in use, like wizard page. >+ // And we only get cached document accessible to destroy, so that we don't >+ // create it just to destroy it. >+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(aTargetNode)); >+ nsCOMPtr<nsIAccessibleDocument> accDoc = GetDocAccessibleFor(doc); >+ if (accDoc) { If we only want to shutdown if the pagehide is on a chrome tab should we do this? nsCOMPtr<nsIDocShellTreeItem> treeItem = nsCoreUtils::GetDocShellTreeItemFor(aTargetNode); PRInt32 itemType; docShellTreeItem->GetItemType(&itemType); if (itemType == nsIDocShellTreeItem::typeChrome) { ... >+ nsRefPtr<nsAccessNode> docAccNode = nsAccUtils::QueryAccessNode(accDoc); >+ docAccNode->Shutdown(); >+ } >+ >+ return NS_OK; >+ } >+} >+

(In reply to comment #14) > Ginn, can we rely on EVENT_DOM_DESTROY instead? (Your comment #1) I don't know > the history here. EVENT_DOM_DESTROY is sent for nsXULTabpanelAccessible, I don't know if there's an easy way to get nsDocAccessible from nsXULTabpanelAccessible. (comment #4) Also if we rely on EVENT_DOM_DESTROY, we still need to listen pagehide in nsRootAccessible. > >+ // pagehide event can be fired under several conditions, such as HTML > >+ // document going away, closing a window/dialog, and wizard page changing. > >+ // We only destroy the accessible object when it's a document accessible, > >+ // so that we don't destroy something still in use, like wizard page. > >+ // And we only get cached document accessible to destroy, so that we don't > >+ // create it just to destroy it. > >+ nsCOMPtr<nsIDocument> doc(do_QueryInterface(aTargetNode)); > >+ nsCOMPtr<nsIAccessibleDocument> accDoc = GetDocAccessibleFor(doc); > >+ if (accDoc) { > > If we only want to shutdown if the pagehide is on a chrome tab should we do > this? > > nsCOMPtr<nsIDocShellTreeItem> treeItem = > nsCoreUtils::GetDocShellTreeItemFor(aTargetNode); > PRInt32 itemType; > docShellTreeItem->GetItemType(&itemType); > if (itemType == nsIDocShellTreeItem::typeChrome) { I think it's not limited to typeChrome.

Ginn, please you answer on comment #13 because I think we don't need to catch pagehide event in this case.

surkov, You're right, if I close a tab with subdocuments, I get n*(n+1)/2 times pagehide. That's not what we want. If I close a window with a doc with subdocuments, I only get 1 pagehide. Hmm. So your suggestion is keep listening "pagehide" in nsRootAccessible, and deal "closing a tab" as an exception case?

In nsDocAccessible::HandleEventWithTarget() if we do if ((mDOMNode == aTargetNode) && (eventType.EqualsLiteral("pagehide"))) { this->Shutdown(); } Would it be simpler and easier to understand?

(In reply to comment #17) > surkov, > You're right, if I close a tab with subdocuments, I get n*(n+1)/2 times > pagehide. What does the event target point to in these events?

(In reply to comment #17) > surkov, > You're right, if I close a tab with subdocuments, I get n*(n+1)/2 times > pagehide. That's not what we want. > If I close a window with a doc with subdocuments, I only get 1 pagehide. Hmm. > > So your suggestion is keep listening "pagehide" in nsRootAccessible, and deal > "closing a tab" as an exception case? Right, though I wouldn't call it as exception case :) I think we want to get pagehide when window is closed because there is no proper mutation events and we do not want pagehide event when there are mutation events (in the case of subdocuments). (In reply to comment #18) > In nsDocAccessible::HandleEventWithTarget() if we do > > if ((mDOMNode == aTargetNode) && (eventType.EqualsLiteral("pagehide"))) { > this->Shutdown(); > } > > Would it be simpler and easier to understand? I'm not sure. Let's see we have two ways in the case of close tab operation and let's compare the logic: 1) listen all pagehide events and deal with mutation events: destroy closed tab content and do not walk into subdocument because we will catch this in pagehide. 2) deal with nutation events: destroy closed tab content and subdocument content (we listen window's pagehide only). I would prefer 2 because this item looks a bit cleaner in understanding. But we need to add debug code to assert if we get wrong pagehide events.

(In reply to comment #19) > (In reply to comment #17) > > surkov, > > You're right, if I close a tab with subdocuments, I get n*(n+1)/2 times > > pagehide. > > What does the event target point to in these events? I think DOM windows of DOM documents (subdocuments).

(In reply to comment #21) > (In reply to comment #19) > > (In reply to comment #17) > > > surkov, > > > You're right, if I close a tab with subdocuments, I get n*(n+1)/2 times > > > pagehide. > > > > What does the event target point to in these events? > > I think DOM windows of DOM documents (subdocuments). I wonder if the chrome check suggestion in comment #14 would work then. Ginn, did you try it?

(In reply to comment #20) surkov, I prefer 2) too. (In reply to comment #22) > I wonder if the chrome check suggestion in comment #14 would work then. Ginn, > did you try it? I'll try. I think we can put the check at the place we set the pagehide listener rather than the callback.

(In reply to comment #23) > (In reply to comment #22) > > I wonder if the chrome check suggestion in comment #14 would work then. Ginn, > > did you try it? > > I'll try. > I think we can put the check at the place we set the pagehide listener rather > than the callback. It's not working as expected. typeChrome means it is a chrome window, I think it is the same thing as nsRootAccessible. I would like to expect something to discriminate document (window/tab) and sub document (frame/iframe).

I'd like to summarize my discussion with surkov on irc. Surkov, please correct me if I misunderstood. Consider we have a Firefox window with several tabs, and frames. The firefox a11y tree is like this. Minefield - frame (nsRootAccessible, A) - scroll pane (nsXULTabpanelAccessible TAB1) - internal frame (nsOuterDocAccessible) - document frame (nsDocAccessible, B) - scroll pane (nsXULTabpanelAccessible TAB2) - internal frame (nsOuterDocAccessible) - document frame (nsDocAccessible, C) ... - internal frame (nsOuterDocAccessible) - document frame (nsDocAccessible, D) 1) The current implementation is to listen "pagehide" event in nsRootAccessible A. Reloading in frame B, C, D will trigger "pagehide" event. The subdocument a11y will be shutdown in this case, it is fine. On closing the browser window, we get "pagehide" event for the top document first, and we shutdown its children docs. The problem is when a tab is closed, we don't receive "pagehide" in nsRootAccessible. 2) The patch moved listening of "pagehide" event from nsRootAccessible to nsDocAccessible. It solved the tab closing issue. But since we have multiple listeners in this case (A,B,C,D), the pagehide of D will propagate to C (and sometimes A). We don't want to deal with the event multiple times. Although it doesn't introduce problems so far. My first thought solution is to listen "pagehide" in B and C, but not A and D. So that we won't receive pagehide several times. On closing TAB2, we get pagehide for C, we shutdown C and D. It's similar to current implementation. Surkov's idea is to keep listening "pagehide" in nsRootAccessible. On closing TAB2, we will receive DOM_DESTORY for TAB2. In RefreshNodes(), we should destroy the sub a11y tree, then we get C and D shutdown. Currently we didn't reach C in RefreshNodes() for TAB2. (possible because RefreshNodes() is a delayed call?)

(In reply to comment #25) > 2) The patch moved listening of "pagehide" event from nsRootAccessible to > nsDocAccessible. > It solved the tab closing issue. > But since we have multiple listeners in this case (A,B,C,D), the pagehide of D > will propagate to C (and sometimes A). Technically we should get events for D and C (I suppose pagehide is fired for every subdocument separately, please correct me if I'm wrong) and if event is propagated then we should gets one more event for C with D original target. > We don't want to deal with the event multiple times. > Although it doesn't introduce problems so far. That's fine. Though we need to really care it can't bring any problem in this case. > > My first thought solution is to listen "pagehide" in B and C, but not A and D. > So that we won't receive pagehide several times. > On closing TAB2, we get pagehide for C, we shutdown C and D. It should work for this case but the logic should be more complex: we should listen event on A because main window is closed and should listen event on D if its frame is reloaded. > It's similar to current implementation. > > Surkov's idea is to keep listening "pagehide" in nsRootAccessible. > On closing TAB2, we will receive DOM_DESTORY for TAB2. > In RefreshNodes(), we should destroy the sub a11y tree, then we get C and D > shutdown. > Currently we didn't reach C in RefreshNodes() for TAB2. (possible because > RefreshNodes() is a delayed call?) I don't think the problem is RefreshNodes is delayed in THIS case, the problem I think is we don't go into subdocument (see http://mxr.mozilla.org/mozilla/source/accessible/src/base/nsDocAccessible.cpp#1763). But delayed RefreshNodes can be a problem. Let's imagine we remove tab2 and then tabbox. We'll coalesce destroy events and we'll save event for tabobox only, in this case we won't achieve subdocuments in RefreshNodes because tab2 is attached from DOM hierarchy of tabbox already.

(In reply to comment #26) > Technically we should get events for D and C (I suppose pagehide is fired for > every subdocument separately, please correct me if I'm wrong) and if event is > propagated then we should gets one more event for C with D original target. That's right. > It should work for this case but the logic should be more complex: we should > listen event on A because main window is closed and should listen event on D if > its frame is reloaded. When frame D is reloaded, the pagehide event is propagated to C. So we don't need to add a listener for D. > I don't think the problem is RefreshNodes is delayed in THIS case, the problem > I think is we don't go into subdocument (see > http://mxr.mozilla.org/mozilla/source/accessible/src/base/nsDocAccessible.cpp#1763). > But delayed RefreshNodes can be a problem. Let's imagine we remove tab2 and > then tabbox. We'll coalesce destroy events and we'll save event for tabobox > only, in this case we won't achieve subdocuments in RefreshNodes because tab2 > is attached from DOM hierarchy of tabbox already. I tried to remove "iterContent->IsInAnonymousSubtree()" But it seems it didn't help.

Ginn, I tried to write mochitests. It should be equivalent to steps of reproducing of this bug but it's not. In this case we get pagehide event for iframe's document. Any idea why is it different?

(In reply to comment #28) > Created an attachment (id=374033) [details] > mochitest1 > > Ginn, I tried to write mochitests. It should be equivalent to steps of > reproducing of this bug but it's not. In this case we get pagehide event for > iframe's document. Any idea why is it different? This bug is only reproducible on closing a tab. AFAIK.

(In reply to comment #29) > (In reply to comment #28) > > Created an attachment (id=374033) [details] [details] > > mochitest1 > > > > Ginn, I tried to write mochitests. It should be equivalent to steps of > > reproducing of this bug but it's not. In this case we get pagehide event for > > iframe's document. Any idea why is it different? > > This bug is only reproducible on closing a tab. AFAIK. Ginn, we need to know the difference because if we'll start to invalidate subdocument on destory of parent accessible and we will get pagehide the same time in some cases then it's not good. I'll try to play tabbrowser to learn where's difference. But now it sounds it's not a11y bug because sometimes pagehide is propagated from unattached DOM content and sometimes is not.

Comment on attachment 367565 [details] [diff] [review] patch cancelling reviews until we get an agreement on approach

probably this is related: test_event_scroll.xul which uses xul:tabbrowser adds root accessible and doesn't remove it. The attached mochitests shows this.

fixed by bug 566103, confirmed by Ginn (bug 566103 comment #35)