193802 - MSAA cache not getting invalidated when new page loads

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The MSAA cache in the Accessible class, is supposed to get invalidated whenever
a new page loads. The new page load events are supposed to force RootAccessible
to do that.

Comment 1

[Aaron Leventhal]

What's happening is that there can be more than 1 accessible for the same
document, because it is deep in the tree. Each may have it's own cache, and the
wrong one can get invalidated.

I have verified that the cache is properly invalidated when an embedding app
like winembed is used -- it's content document is the top root accessible, so
this situation cannot occur.

The solution is to make sure we only ever have 1 accessible for each document.

See also bug 180415, which needs a similar fix.

Comment 2

[Aaron Leventhal]

Attachment: Compiles on Windows. Not ready for review.

Bolian, I'm going to work to make this build with gtk2 next. You can take a
look at it if you want to see what I'm doing.

Comment 3

[Aaron Leventhal]

Attachment: Still only compiles on windows, diff'd against current trunk

Comment 4

[Aaron Leventhal]

Kyle, Bolian -

You can start reviewing this now. So far it only compiles on Windows, but 99% of
the patch is ready. I'll add the fixes to make it compile with Mai later.

I shuld be online around midnight California time to answer questions.

Comment 5

[Aaron Leventhal]

Attachment: Fixing merge conflicts due to Bryner's MOZ_XUL checkin

Comment 6

[Aaron Leventhal]

Attachment: Builds with Mai

Bolian, Kyle - sorry this breaks Mai's event handling. In the new system you'll
handle that in nsDocAccessibleWrap::FireToolkitEvent().

Also, this patch fixes accessibility to use it's own atom table, so it doesn't
have to link with gkconshared.

Comment 7

[Aaron Leventhal]

Comment on attachment 120180 [details] [diff] [review]
Builds with Mai

Okay, bring on the critique . I'm ready :-)

Comment 8

[Kyle Yuan]

Aaron, hopefully I can get this done by the weekend. if I can't, I'll do that as
the first thing on next Monday.

Comment 9

[Aaron Leventhal]

Okay Kyle -- I am leaving California next Thursday morning (first to Chicago for
5 days, then to Germany for 6 months).

Comment 10

[Aaron Leventhal]

Attachment: Fixes problems found after testing plugins, events and buttons

Comment 11

[Kyle Yuan]

Comment on attachment 120256 [details] [diff] [review]
Fixes problems found after testing plugins, events and buttons

r=kyle. The code clean up is great. I like it! 

two nits:
1) accessible/src/base/nsFormControlAccessible.cpp
-#include "nsString.h"
+#include "nsIDOMHTMLInputElement.h"#include "nsIDOMXULElement.h"
+#include "nsIDOMXULControlElement.h"
two include statements in the same line.

2) accessible/src/base/nsAccessibilityAtomList.h
+ * Portions created by the Initial Deveper are Copyright (C) 2003
typo: Deveper->Developer

Comment 12

[Aaron Leventhal]

Attachment: new patch with kyle's nits, and fixes a problem with menu focus events

Comment 13

[Aaron Leventhal]

Attachment: Fix error return code for FireToolkitEvent

Comment 14

[Alec Flett]

Comment on attachment 120416 [details] [diff] [review]
Fix error return code for FireToolkitEvent

+      QueryInterface(NS_GET_IID(nsISupportsWeakReference),
getter_AddRefs(supportsWeak));
+      nsCOMPtr<nsIWeakReference> weakShell(do_GetWeakReference(supportsWeak));

huh? do you want the weak reference or not? 

I think this should be
nsWeakPtr supportsWeak = do_GetWeakReference(this);

(use NS_STATIC_CAST(nsIPresShell*, this)) if you get casting errors from the
compiler.

I find it a little odd that you're supposed to pass in nsIWeakReference* to
GetAccessibleInShell... why not pass in the raw nsIPresShell... why burden the
consumers of GetAccessibleInShell by making them create the weak ref
themselves?

 In fact, I'm seeing that GetAccessibleInShell just queries back out of the
weak reference! I don't understand this at all.

Look at other code for examples of how to use weak refs - you don't need them
here.

Comment 15

[Aaron Leventhal]

Comment on attachment 120416 [details] [diff] [review]
Fix error return code for FireToolkitEvent

Alec, 99% of the callers GetAccessibleInShell have already have a weak pointer
to the shell as an mPresShell member variable. That's because most of the
callers are other accessible classes which need a weak pointer to the pres
shell.

The only place that calls GetAccessibleInShell which doesn't have the weak
reference at the ready, is in nsPresShell. It makes things simpler to take a
weak reference as an argument.

Also, I had already tried
nsWeakPtr supportsWeak = do_GetWeakReference(this);
Unforutnately, this causes a compiler error
error C2594: 'argument' : ambiguous conversions from 'class PresShell *const '
to 'class nsISupports *'

So is it okay to leave it this way?

Comment 16

[Alec Flett]

[16:04:41] <alecf> to fix that compiler error, you need to do this
[16:04:54] <alecf> nsWeakPtr foo =
do_GetWeakReference(NS_STATIC_CAST(nsIPresShell*,this));
[16:05:22] <alecf> and as for the weak ref stuff, why not add another method to
the interface that doesn't take a weak pointer, that does the work of getting
the weak pointer
[16:05:44] <alecf> so that that code only exists once, to get the weak reference
[16:06:03] <alecf> instead of scattering it throughout the source

Comment 17

[Aaron Leventhal]

Attachment: New patch with alecf's suggested fixes -- has GetAccesibleInShell() and GetAccessibleInWeakShell()

I also changed our member variable names to mWeakShell instead of mPresShell
where it's really a weak reference, so that it's more clear.

Comment 18

[Aaron Leventhal]

Comment on attachment 120514 [details] [diff] [review]
New patch with alecf's suggested fixes -- has GetAccesibleInShell() and GetAccessibleInWeakShell()

Carrying r= forwarding

Comment 19

[Alec Flett]

Comment on attachment 120514 [details] [diff] [review]
New patch with alecf's suggested fixes -- has GetAccesibleInShell() and GetAccessibleInWeakShell()

+already_AddRefed<nsIPresShell> nsAccessNode::GetPresShell()
+{
+  nsCOMPtr<nsIPresShell> presShell(do_QueryReferent(mWeakShell));
+  if (!presShell) {
+    if (mWeakShell) {
+      Shutdown();
+    }
+    return nsnull;
+  }

I'm sure there's good reason for this logic, but its not obvious to me...

+#ifdef OLD_HASH
+void nsAccessNode::GetDocAccessibleFor(nsIWeakReference *aPresShell, 
+					nsIAccessibleDocument **aDocAccessible)

why keep OLD_HASH stuff around? lets dump it if you know the nsTHashtable stuff
works.

+  NS_IF_RELEASE(gStringBundle);
+  NS_IF_RELEASE(gKeyStringBundle);
+  NS_IF_RELEASE(gLastFocusedNode);
+
+  // And just to be safe
+  gStringBundle = gKeyStringBundle = nsnull;
+  gLastFocusedNode = nsnull;

you don't need to reset these to nsnull again - NS_*_RELEASE does that for you.

I don't know if this is going to break the MOZ_XUL that just landed, but if you
have cycles, I would highly recommend trying to build with --disable-xul

the rest looks good
sr=alecf with the above changes..

Comment 20

[Aaron Leventhal]

Fixed. Will file separate bug to get THashTable working, so we can remove the 
parts that are #ifdef OLD_HASH

Comment 21

[Boris Zbarsky [:bzbarsky]]

Well, in the meantime the OLD_HASH code leaks in
nsAccessNode::GetDocAccessibleFor.  In particular:

  nsCOMPtr<nsIAccessNode> accessNode = NS_STATIC_CAST(nsIAccessNode*,
gGlobalDocAccessibleCache->Get(&key));

Congrats, you just leaked -- nsSupportsHashtable::Get() stupidly addrefs the
pointer before returning.

The move to nsTHashtable may help with this, but you might want to fix it for
now in any case...

Comment 22

[Aaron Leventhal]

In the middle of moving to Germany for 6 months. Won't be able to get to it for
at least a week.

Kyle, if you have tiem to take a look at this leak, or the spinoff bug 202080,
that would be great.

Comment 23

[Boris Zbarsky [:bzbarsky]]

btw, just doing:

 nsCOMPtr<nsIAccessNode> accessNode = dont_AddRef(NS_STATIC_CAST(nsIAccessNode*,
gGlobalDocAccessibleCache->Get(&key)));

will fix the leak...

Comment 24

[Kyle Yuan]

sorry, aaron, I'm stumped by another project...
bz, I'm okay with checking this small fix in :)

Comment 25

[Aaron Leventhal]

Boris, I don't think it actually leaks. I tested with leak.log before checking
in and again now.

  nsCOMPtr<nsIAccessNode> accessNode = NS_STATIC_CAST(nsIAccessNode*,
gGlobalDocAccessibleCache->Get(&key));
  nsCOMPtr<nsIAccessibleDocument> accDoc(do_QueryInterface(accessNode));
  *aDocAccessible = accDoc;  // already addrefed

On the last line, I comment that we're already addref'd. That's why I didn't use
NS_IF_ADDREF in this getter.

Comment 26

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381