571459 - shutdown document accessible when presshell goes away

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

We listen DOM pagehide events and style changes notifications from layout to shutdown document accessible. We could remove these listeners if presshell notifies us it's destroyed. As well it fix the problem we get style changes notifications after presshell destruction (per bug 563327 comment #33).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

I don't think we can nix the pagehide listeners, since presshells don't die when the document goes into bfcache.  On the other hand, maybe we want to bfcache accessibles?

Not sure what other code we could remove here.

Comment 2

[alexander :surkov (:asurkov)]

bfcache is about when tab is closed? If so then I think it makes sense to not recreate the accessible tree.

Comment 3

[alexander :surkov (:asurkov)]

Also it's needed to remove ShutdownDocAccessibleTree from nsOuterDocAccessible, since it sounds excess. nsOuterDocAccessible::Shutdown() is called when we are notified about display style changes for iframe (here pagehide doesn't trigger if I get right).

Comment 4

[Boris Zbarsky [:bzbarsky]]

> bfcache is about when tab is closed?

No, it's about navigating away from the page but caching the presentation so "back" can happen quickly.

> Also it's needed to remove ShutdownDocAccessibleTree from nsOuterDocAccessible,

Looking into this.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: With that change

I assume that nsAccessible::InvalidateChildren on the outer doc accessible is ok even if the document accessible inside has already been shut down?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Created an attachment (id=450704) [details]
> With that change
> 
> I assume that nsAccessible::InvalidateChildren on the outer doc accessible is
> ok even if the document accessible inside has already been shut down?

Yes, it's ok. nsAccDocManager add document accessible as a child to iframe accessible, and document accessible removes itself from iframe accessible when it shutdowns. So basicly InvalidateChildren() makes mostly nothing for iframe accessible in opposite to other accessibles.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> > bfcache is about when tab is closed?
> 
> No, it's about navigating away from the page but caching the presentation so
> "back" can happen quickly.

I think it would be really nice to preserve accessible tree in this case. And then we don't need pagehide events, right? However we need to unattach the document accessible form the tree when it happens and attach it again when the user returns to the page.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Sure; I think that should be a separate bug, though.

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 450704 [details] [diff] [review]
With that change

 
>+  /**
>+   * Shutdown the document accessible.
>+   */
>+  void ShutdownDocAccessible(nsIDocument *aDocument);

then ShutdownDocAccessibleInTree shouldn't be public any more.


>+nsAccessibilityService::PresShellDestroyed(nsIPresShell* aPresShell)
>+{
>+  //  Presshell destruction will automatically destroy shells for

nit: two line spaces
>+  //  descendant documents, so no need to worry about those.  Just
>+  //  shut down the accessible for this one document.  That keeps us
>+  //  from having bad behavior in case of deep bushy subtrees.
>+  nsIDocument* doc = aPresShell->GetDocument();
>+  if (doc) {

nit: no { } around single if.

>+    ShutdownDocAccessible(doc);

I don't insist to fix these but it's commonly used style in a11y module

> nsresult
> nsOuterDocAccessible::Shutdown()
> {
>-  // Shutdown child document if any.
>-  nsAccessible *childAcc = mChildren.SafeElementAt(0, nsnull);
>-  if (childAcc) {
>-    nsRefPtr<nsDocAccessible> docAcc(do_QueryObject(childAcc));
>-    NS_LOG_ACCDOCDESTROY_FOR("outerdoc document shutdown",
>-                             docAcc->GetDOMDocument(), docAcc.get())
>-    GetAccService()->ShutdownDocAccessiblesInTree(docAcc->GetDOMDocument());
>-  }
>-
>   nsAccessible::InvalidateChildren();

I really don't recall why I've added it but it shouldn't be needed because it clears children array but document accessible removes itself from children when it shutdowns.

thank you! r=me

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Sure; I think that should be a separate bug, though.

sure

Comment 11

[Boris Zbarsky [:bzbarsky]]

> then ShutdownDocAccessibleInTree shouldn't be public any more.

Good catch.  Done.

> nit: no { } around single if.

Done.

> I really don't recall why I've added it but it shouldn't be needed 

OK.  I took it out.

Comment 12

[Drew Willcoxon :adw]

This landed here:
http://hg.mozilla.org/mozilla-central/rev/b54140961fa7

But I backed it out here:
http://hg.mozilla.org/mozilla-central/rev/7523ad2f395e

Because of some persistent test crashes after http://hg.mozilla.org/mozilla-central/rev/4dc9a5d9167b landed, e.g.:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276556957.1276557785.11024.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276554608.1276555936.1277.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276555880.1276557939.11978.gz

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276557545.1276559400.19509.gz

Comment 13

[Boris Zbarsky [:bzbarsky]]

More precisely, timeouts of test_docload.xul, not crashes.

Looking into that, I see:

###!!! ASSERTION: Previous child document of outerdoc accessible wasn't removed!: '!mChildren.Length()', file ../../../../mozilla/accessible/src/base/nsOuterDocAccessible.cpp, line 184

a bunch of times, then the test popup window shows an error page:

  Firefox can't find the server at www.wronguri.wronguri.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I've confirmed that it's the shutdown call the newly added hook makes that causes the problem.  I have also confirmed that the assertion from comment 13 appears the first time before the hook actually shuts down anything in the test...

At that point, the accessible being added is for about:robots while the one that's already in mChildren is for about:blank.  

Alexander, any idea what might be going on here?

Comment 15

[alexander :surkov (:asurkov)]

It sounds presshell is destroyed after "pagehide" event was handled and I suspect different documents can share the same document->GetWindow()->GetChromeEventHandler() so presshell destroy for "early" document removes "pagehide" events handlers, therefore we don't handle them and don't destroy accessible document for "late" document. When we get presshell destroy for "late" document, new document has been created and attached to outerdoc accessible already. If we get "pagehide" events before presshell is  destroyed then it sounds this bug allows to get rid special shutdown documents handling in outerdoc accessible.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> Created an attachment (id=451265) [details]
> patch

It makes fail:

2204 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/relations/test_general.xul | [xpconnect wrapped (nsISupports, nsIAccessible, nsIAccessNode) @ 0x10cce8e0 (native @ 0x13016f68)] is not a target of relation of 'node child of' type for '[ 'document node' , role: document, name: 'about:blank']'.
2205 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/relations/test_general.xul | There is unexpected target[no node infodefunct]of relation of 'node child of' type for '[ 'document node' , role: document, name: 'about:blank']'.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> It sounds presshell is destroyed after "pagehide" event was handled

Yes, this would be quite common for navigation.

> I suspect different documents can share the same
> document->GetWindow()->GetChromeEventHandler()

Yes, of course.  In some Gecko embeddings, that might be a singleton so that _all_ documens share it.  What are we doing with the chrome event handler, exactly?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Yes, this would be quite common for navigation.

At least if bfcache is involved.  Not sure about the other case....

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #18)
 
> > I suspect different documents can share the same
> > document->GetWindow()->GetChromeEventHandler()
> 
> Yes, of course.  In some Gecko embeddings, that might be a singleton so that
> _all_ documens share it.  What are we doing with the chrome event handler,
> exactly?

That's was historically. What the target should be for pagehide event?

(In reply to comment #19)
> > Yes, this would be quite common for navigation.
> 
> At least if bfcache is involved.  Not sure about the other case....

I don't think we test back/forward navigation here if that's the case of bfcache.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> What the target should be for pagehide event?

What do you mean?  The .target of the event is set to the document, but the event is dispatched to the window, if I recall correctly.  Observing the chrome event handler is ok as long as we check for the right target.  Or you could add the handler to the window, and then I think you might not need the target check.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch2

relations/test_generic.xul failed because we were notified of layout style change for xul:iframe (node_child_of relation test) what makes us shutdown the outerdoc accessible for iframe, however the presshell of content document isn't destroyed. We need to shutdown content document accessible to avoid "hanging" document accessible.

Comment 23

[alexander :surkov (:asurkov)]

try server build http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-29121cdfe1c2/

Comment 24

[Marco Zehe (:MarcoZ)]

Comment on attachment 451475 [details] [diff] [review]
patch2

This patch is working for me. Different screen readers, I tested tabs, inline frames, simple and complex docs, dynamic changes like on Twitter when hovering over a tweet, opening and closing tabs, going back and forward through the history of a tab etc.

Comment 25

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

Comment on attachment 451475 [details] [diff] [review]
patch2

r=me. I didn't catch any code errors.

Normally I would feel better removing the listeners explicitly but I trust this should work.

Glad to see can now get PresShellDestroyed calls. Nice one guys.

(My mind wandered a bit to content process separation, events, and where a11y will live.)

>+//#define DEBUG_ACCDOCMGR

I like this BTW. I think we have too much logging in DEBUG and should specialize more of our logging to reduce noise.

>+  nsIDocument *GetDocumentNode() const

nit: I much prefer nsIDocument* (no space) to more obviously show the return type is a pointer.

> void
> nsOuterDocAccessible::Shutdown()
> {
>-  // Shutdown child document if any.
>+  // XXX: sometimes outerdoc accessible is shutdown because of layout style
>+  // change however the presshell of underlying document isn't destroyed and
>+  // the document doesn't get pagehide events. Shutdown underlying document if
>+  // any to avoid hanging document accessible.

Hmm ok.

>+var gA11yEventDumpToConsole = false;

Handy :)

>+++ b/layout/base/nsPresShell.cpp
>+#ifdef ACCESSIBILITY
>+  if (gIsAccessibilityActive) {
>+    nsCOMPtr<nsIAccessibilityService> accService =
>+      do_GetService("@mozilla.org/accessibilityService;1");
>+    if (accService) {
>+      accService->PresShellDestroyed(this);
>+    }
>+  }
>+#endif // ACCESSIBILITY
>+

Nice to see this. I hope we don't fire any sync events during the PresShellDestroyed call.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> nit: I much prefer nsIDocument* (no space) to more obviously show the return
> type is a pointer.

Ok, it goes with Mozilla style guide - https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide#Declarations, I'm fine with this.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #25)

> Nice to see this. I hope we don't fire any sync events during the
> PresShellDestroyed call.

We don't fire any event.

Comment 28

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

Attachment: Obj-C exception (events/test_docload.html)

Consistent exception on OSX when running events/test_docload.html.

Comment 29

[alexander :surkov (:asurkov)]

Unfortunately I don't have any idea what can be wrong. I need to debug. Perhaps the best way is to file bug for this so that I can get back to it later when I get a mac.

Comment 30

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

(In reply to comment #29)
> Unfortunately I don't have any idea what can be wrong. I need to debug. Perhaps
> the best way is to file bug for this so that I can get back to it later when I
> get a mac.

OK.  (... and I can take a look but probably not this week)

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)

> OK.  (... and I can take a look but probably not this week)

that would be great, thank you.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 451475 [details] [diff] [review]
patch2

>+  // XXX: sometimes outerdoc accessible is shutdown because of layout style
>+  // change however the presshell of underlying document isn't destroyed 

Uh.. that shouldn't happen.  Did it happen for you?

Also, the debug output should just use %p instead of random casts to int, so it works in 64-bit builds.

Holding off on the sr for now pending explanation of that XXX comment.

Comment 33

[alexander :surkov (:asurkov)]

(In reply to comment #32)
> (From update of attachment 451475 [details] [diff] [review])
> >+  // XXX: sometimes outerdoc accessible is shutdown because of layout style
> >+  // change however the presshell of underlying document isn't destroyed 
> 
> Uh.. that shouldn't happen.  Did it happen for you?

yes, it happens in http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/relations/test_general.xul#62 when tests are run all together, however if I run test in standalone mode then there's no fail.

iirc there two loads of about:blank into for xul:iframe happens there, between them iframe accessible is recreated (I think this is because of layout notifications), old about:blank isn't shutdown and therefore I get old doc accessible when I ask for an accessible for iframe's document.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> between them iframe accessible is recreated (I think this is because of layout
> notifications)

Are these notifications that don't destroy the nsIFrame involved?  If the nsIFrame dies, the presshell inside should really die too.... and if the document had no presshell it wouldn't have an accessible either, right?

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)
> > between them iframe accessible is recreated (I think this is because of layout
> > notifications)
> 
> Are these notifications that don't destroy the nsIFrame involved?  If the
> nsIFrame dies, the presshell inside should really die too.... 

at least the document isn't shutdown what means nsIAccessibilityService::destroyPressShell() doesn't trigger (nor pagehide event as well).

Comment 36

[Boris Zbarsky [:bzbarsky]]

Right.  Which is what I can't understand.  If a document has a presshell (which is required for us to create an accessible), then the presshell _will_ get destroyed at some point.  Is the problem here that something else happens before that presshell is destroyed?

Comment 37

[alexander :surkov (:asurkov)]

presshell isn't destroyed, it sounds we get excess layout change notification we shouldn't process on our side.

Comment 38

[alexander :surkov (:asurkov)]

Attachment: log (bug)

Comment 39

[alexander :surkov (:asurkov)]

Attachment: log (nobug)

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch3

fixed style nits and reinterpret_cast<int> is replaced on %p

Boris, I think we should deal with outerdoc accessible invalidation problem separately, it loos like we get layout notification for iframe and recreate outerdoc accessible when it's not necessary.

Comment 41

[alexander :surkov (:asurkov)]

Attachment: hide iframe test

removal document on hide iframe doesn't trigger "pagehide" listeners removal since it's destroyed silently from RefreshNodes(). On another hand I suspect this silently removed document accessible isn't removed from document cache.

Comment 42

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

Comment on attachment 451927 [details] [diff] [review]
hide iframe test

>+NS_IMETHODIMP
>+nsAccessibilityService::GetAccessibleFromCache(nsIDOMNode* aNode,
>+                                               nsIAccessible** aAccessible)
>+{
>+  NS_ENSURE_ARG_POINTER(aAccessible);
>+
>+  // Search an accessible in caches of document accessibles, if failed and
>+  // the node is a document then check the document cache. Usually if document
>+  // accessible is shutdown then it isn't stored in document cache however
>+  // "unofficially" shutdown document isn't removed from document cache.

Can you describe what "unofficially" means here?

> /**
>+ * Return true if accessible for the given node is in cache.
>+ */
>+function isAccessibleInCache(aNodeOrId)
>+{
>+  var node = getNode(aNodeOrId);
>+  return gAccRetrieval.getAccessibleFromCache(node) ? true : false;

(This could be done in one line of course)

>+++ b/accessible/tests/mochitest/events/docload_wnd.html
>@@ -0,0 +1,22 @@
>+<html>
>+
>+<head>

Nit: don't need extra line space

>diff --git a/accessible/tests/mochitest/events/test_docload.html b/accessible/tests/mochitest/events/test_docload.html

>+        ok(!isAccessibleInCache(dlgDoc),
>+           "The document accessible for '" + aURL + "' is in cache still!");
>       }

Does this fail without your patch?


>+    function openWndShutdownDoc()
>+    {
>+      this.__proto__ =
>+        new openDialogWnd("chrome://mochikit/content/a11y/accessible/events/docload_wnd.html");
>+
>+      var thisObj = this;
>+      var docChecker = {
>+        type: EVENT_HIDE,
>+        get target()
>+        {

"get target"?

Comment 43

[alexander :surkov (:asurkov)]

(In reply to comment #42)
> (From update of attachment 451927 [details] [diff] [review])
> >+NS_IMETHODIMP
> >+nsAccessibilityService::GetAccessibleFromCache(nsIDOMNode* aNode,
> >+                                               nsIAccessible** aAccessible)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aAccessible);
> >+
> >+  // Search an accessible in caches of document accessibles, if failed and
> >+  // the node is a document then check the document cache. Usually if document
> >+  // accessible is shutdown then it isn't stored in document cache however
> >+  // "unofficially" shutdown document isn't removed from document cache.
> 
> Can you describe what "unofficially" means here?

Not from nsAccDocManager, for example, from IsDefunct() when we discovered presshell has gone but we aren't notified yet.
> >+function isAccessibleInCache(aNodeOrId)
> >+{
> >+  var node = getNode(aNodeOrId);
> >+  return gAccRetrieval.getAccessibleFromCache(node) ? true : false;
> 
> (This could be done in one line of course)

I don't mind what do you prefer?

> >+        ok(!isAccessibleInCache(dlgDoc),
> >+           "The document accessible for '" + aURL + "' is in cache still!");
> >       }
> 
> Does this fail without your patch?

no
> >+        get target()
> >+        {
> 
> "get target"?

what's wrong here?

Comment 44

[Boris Zbarsky [:bzbarsky]]

> I think we should deal with outerdoc accessible invalidation problem separately

Sounds good.

Comment 45

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

Comment on attachment 451927 [details] [diff] [review]
hide iframe test

OK r=me.

Can you add to your comment about "unofficial" -> "unofficial (Not from nsAccDocManager)"

Two line impl for isAccessibleInCache is fine.

>-    // var gA11yEventDumpID = "eventdump"; // debug stuff
>+    var gA11yEventDumpID = "eventdump"; // debug stuff

Don't forget to recomment this :)

BTW I had to ask Joe about the 'get' keyword... interesting.

Comment 46

[alexander :surkov (:asurkov)]

Attachment: hide iframe test2

Comment 47

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

Comment on attachment 451966 [details] [diff] [review]
hide iframe test2

>+   * Return an accessible for the given DOM node from the cache.
>+   * @note  the method is intended for testing purposes

>+  nsIAccessible getAccessibleFromCache(in nsIDOMNode aNode);

Actually I wonder if we should mark this deprecated so that people don't use it.

>+  // Search an accessible in caches of document accessibles, if failed and
>+  // the node is a document then check the document cache. Usually if document
>+  // accessible is shutdown then it isn't stored in document cache however
>+  // "unofficially" shutdown document (i.e. not from nsAccDocManager) isn't
>+  // removed from document cache.

Maybe:
Search for an accessible in each of our per document accessible object caches. If we don't find it, and the given node is itself a document, check our cache of document accessibles (document cache). Note usually shutdown document accessibles are not stored in the document cache, however an "unofficially" shutdown document (i.e. not from nsAccDocManager) can still exist in the document cache.

>diff --git a/accessible/tests/mochitest/common.js b/accessible/tests/mochitest/common.js
> 
>+  acc.QueryInterface(nsIAccessNode);
>+

Why this call?

>diff --git a/accessible/tests/mochitest/events/test_docload.html b/accessible/tests/mochitest/events/test_docload.html
>       this.finalCheck = function openDialogWnd_finalCheck()
>       {
>+        this.finalCheckImpl();
>+      }
>+
>+      this.finalCheckImpl = function openDialogWnd_finalCheckImpl()
>+      {

Why not just put the implementation in finalCheck?

Comment 48

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

Comment on attachment 451966 [details] [diff] [review]
hide iframe test2

>+      this.finalCheck = function()
>+      {
>+        // After timeout after event hide for iframe was handled the document
>+        // accessible for iframe's document is in cache still.
>+        todo(!isAccessibleInCache(this.iframeDoc),
>+             "The document accessible for iframe is in cache still!");
>+
>+        this.finalCheckImpl();

Ah I see.

Ok pretty complex, but r=me.

Comment 49

[alexander :surkov (:asurkov)]

(In reply to comment #47)
> (From update of attachment 451966 [details] [diff] [review])
> >+   * Return an accessible for the given DOM node from the cache.
> >+   * @note  the method is intended for testing purposes
> 
> >+  nsIAccessible getAccessibleFromCache(in nsIDOMNode aNode);
> 
> Actually I wonder if we should mark this deprecated so that people don't use
> it.

I think no, it should be handy method for tools.

> >diff --git a/accessible/tests/mochitest/common.js b/accessible/tests/mochitest/common.js
> > 
> >+  acc.QueryInterface(nsIAccessNode);
> >+
> 
> Why this call?

to make sure returned accessible from getAccessible() is queried to nsIAccessNode automatically.

Comment 50

[alexander :surkov (:asurkov)]

test landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/31d4d880d0a8

patch landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/279ce7f9b1e3

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #44)
> > I think we should deal with outerdoc accessible invalidation problem separately
> 
> Sounds good.

I filed bug 572952 for this