MSAA cache not getting invalidated when new page loads

RESOLVED FIXED

Status

()

RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

({access, sec508})

Trunk
x86
Windows XP
access, sec508
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Comment 1

16 years ago
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.
(Assignee)

Comment 2

16 years ago
Created attachment 120128 [details] [diff] [review]
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.
(Assignee)

Comment 3

16 years ago
Created attachment 120159 [details] [diff] [review]
Still only compiles on windows, diff'd against current trunk
Attachment #120128 - Attachment is obsolete: true
(Assignee)

Comment 4

16 years ago
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.
(Assignee)

Comment 5

16 years ago
Created attachment 120167 [details] [diff] [review]
Fixing merge conflicts due to Bryner's MOZ_XUL  checkin
(Assignee)

Updated

16 years ago
Attachment #120159 - Attachment is obsolete: true
(Assignee)

Comment 6

16 years ago
Created attachment 120180 [details] [diff] [review]
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.
(Assignee)

Comment 7

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

Okay, bring on the critique . I'm ready :-)
Attachment #120180 - Flags: review?(kyle.yuan)

Comment 8

16 years ago
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.
Status: NEW → ASSIGNED
(Assignee)

Comment 9

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

Comment 10

16 years ago
Created attachment 120256 [details] [diff] [review]
Fixes problems found after testing plugins, events and buttons
Attachment #120180 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #120180 - Flags: review?(kyle.yuan)
(Assignee)

Updated

16 years ago
Attachment #120256 - Flags: review?(kyle.yuan)

Comment 11

16 years ago
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
Attachment #120256 - Flags: review?(kyle.yuan) → review+
(Assignee)

Comment 12

16 years ago
Created attachment 120413 [details] [diff] [review]
new patch with kyle's nits, and fixes a problem with menu focus events
Attachment #120256 - Attachment is obsolete: true
(Assignee)

Comment 13

16 years ago
Created attachment 120416 [details] [diff] [review]
Fix error return code for FireToolkitEvent
(Assignee)

Updated

16 years ago
Attachment #120413 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #120416 - Flags: review?(kyle.yuan)

Updated

16 years ago
Attachment #120416 - Flags: review?(kyle.yuan) → review+
(Assignee)

Updated

16 years ago
Attachment #120416 - Flags: superreview?(alecf)

Comment 14

16 years ago
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.
Attachment #120416 - Flags: superreview?(alecf) → superreview-
(Assignee)

Comment 15

16 years ago
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?
Attachment #120416 - Flags: superreview- → superreview?

Comment 16

16 years ago
[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
(Assignee)

Comment 17

16 years ago
Created attachment 120514 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Attachment #120416 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Carrying r= forwarding
Attachment #120514 - Flags: superreview?(alecf)
Attachment #120514 - Flags: review+

Comment 19

16 years ago
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..
Attachment #120514 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 20

16 years ago
Fixed. Will file separate bug to get THashTable working, so we can remove the 
parts that are #ifdef OLD_HASH
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
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...
(Assignee)

Comment 22

16 years ago
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.
btw, just doing:

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

will fix the leak...

Comment 24

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

Comment 25

16 years ago
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.
Attachment #120416 - Flags: superreview?
You need to log in before you can comment on or make changes to this bug.