Closed Bug 571459 Opened 10 years ago Closed 10 years ago

shutdown document accessible when presshell goes away

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(6 files, 4 obsolete files)

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).
Attached patch Proposed fix (obsolete) — Splinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #450693 - Flags: review?(surkov.alexander)
Attachment #450693 - Flags: review?(roc)
Blocks: 563327
bfcache is about when tab is closed? If so then I think it makes sense to not recreate the accessible tree.
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).
> 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.
Attached patch With that changeSplinter Review
I assume that nsAccessible::InvalidateChildren on the outer doc accessible is ok even if the document accessible inside has already been shut down?
Attachment #450693 - Attachment is obsolete: true
Attachment #450704 - Flags: review?(surkov.alexander)
Attachment #450704 - Flags: review?(roc)
Attachment #450693 - Flags: review?(surkov.alexander)
Attachment #450693 - Flags: review?(roc)
(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.
(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.
Sure; I think that should be a separate bug, though.
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
Attachment #450704 - Flags: review?(surkov.alexander) → review+
(In reply to comment #8)
> Sure; I think that should be a separate bug, though.

sure
> 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.
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.
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?
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.
Attached patch patch (obsolete) — Splinter Review
(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']'.
> 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?
> Yes, this would be quite common for navigation.

At least if bfcache is involved.  Not sure about the other case....
(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.
> 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.
Attached patch patch2 (obsolete) — Splinter Review
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.
Attachment #451265 - Attachment is obsolete: true
Attachment #451475 - Flags: superreview?(bzbarsky)
Attachment #451475 - Flags: review?(bolterbugz)
Attachment #451475 - Flags: feedback?(marco.zehe)
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.
Attachment #451475 - Flags: feedback?(marco.zehe) → feedback+
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.
Attachment #451475 - Flags: review?(bolterbugz) → review+
(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.
(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.
Consistent exception on OSX when running events/test_docload.html.
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.
(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)
(In reply to comment #30)

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

that would be great, thank you.
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.
(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.
> 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?
(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).
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?
presshell isn't destroyed, it sounds we get excess layout change notification we shouldn't process on our side.
Attached file log (bug)
Attached file log (nobug)
Attached patch patch3Splinter Review
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.
Attachment #451475 - Attachment is obsolete: true
Attachment #451873 - Flags: superreview?(bzbarsky)
Attachment #451475 - Flags: superreview?(bzbarsky)
Attached patch hide iframe test (obsolete) — Splinter Review
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.
Attachment #451927 - Flags: review?(bolterbugz)
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"?
(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?
Attachment #451861 - Attachment mime type: application/octet-stream → text/plain
Attachment #451862 - Attachment mime type: application/octet-stream → text/plain
> I think we should deal with outerdoc accessible invalidation problem separately

Sounds good.
Attachment #451873 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #451927 - Flags: review?(bolterbugz) → review+
Attachment #451927 - Attachment is obsolete: true
Attachment #451966 - Flags: review?(bolterbugz)
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?
Attachment #451966 - Flags: review?(bolterbugz) → review+
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.
(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.
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #44)
> > I think we should deal with outerdoc accessible invalidation problem separately
> 
> Sounds good.

I filed bug 572952 for this
You need to log in before you can comment on or make changes to this bug.