678429 - implement weak reference for accessible objects

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

We shouldn't keep accessible alive when it's about to go away. This is good alternative to IsDefunct and more memory friendly.

It should be used for
# accessible events
# single accessible iterator
# XPCOM objects

Comment 1

[alexander :surkov (:asurkov)]

(In reply to alexander surkov from comment #0)

> # XPCOM objects

Approach for XPCOM objects is to keep own hash to get XPCOM object by given accessible object so it can be sort of own weak reference implementation, i.e. when accessible goes away then it's removed from hash and accessible pointer in XPCOM object is null out.

Having said this approach can be used as a base for common weak reference implementation, i.e. use hash to get weak reference list by accessible object.

For that nsDocAccessible::mAccessibleCache can provide values:
struct
{
  nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle
  AccWeakReferenceList mWeakRefList; // list of weak references
}

Make XPCOM object inherited from weak reference class and keep as the first item of the weak reference list (downcast it to XPCOM object when requested).

Comment 2

[Trevor Saunders (:tbsaunde)]

> # accessible events

as an alternative here what if when we shutdown an accessible we run through the event queue and drop all but a hide event for the accessible on the floor.  since by the time we say this accessibles state changed we'll also say this accessible is dead.

> # single accessible iterator

do we want to weak ref this or just say it can only be used in place where tree is not allowed to change (so probably just getting relations)?

> For that nsDocAccessible::mAccessibleCache can provide values:
> struct
> {
>   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle

It'd be nice if the parent did this, or just the doc itself.

>   AccWeakReferenceList mWeakRefList; // list of weak references
> }
> 
> Make XPCOM object inherited from weak reference class and keep as the first
> item of the weak reference list (downcast it to XPCOM object when requested).

how is  invalidation going to work? virtual function? (simple but not very nice)?
other than the downcasting for xpcom object we could store member pointers I suppose.

Comment 3

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

I'm not sure I have the whole picture yet (or I've forgotten). Are we talking about have an nsWeakAccessible? (similar to the nsWeakFrame setup)

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > # accessible events
> 
> as an alternative here what if when we shutdown an accessible we run through
> the event queue and drop all but a hide event for the accessible on the
> floor.  since by the time we say this accessibles state changed we'll also
> say this accessible is dead.

hide events are not exception but anyway when you shutdown the accessible tree of m objects and has n events in the queue, that's it's n*m. Proposed weak reference approach is faster since it takes n.

> > # single accessible iterator
> 
> do we want to weak ref this or just say it can only be used in place where
> tree is not allowed to change (so probably just getting relations)?

I'm not sure what you mean by "where tree is not allowed to change". Currently iterators are not a problem but new IA2 probably make us to expose this object externally. So we don't control its life cycle and don't want to keep defunct accessible alive.

> > For that nsDocAccessible::mAccessibleCache can provide values:
> > struct
> > {
> >   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle
> 
> It'd be nice if the parent did this, or just the doc itself.

parent probably no because I don't see anything bad in parent but its children recreation. Document yes, and that's what I proposed actually (since mAccessibleCache is managed by document).

> >   AccWeakReferenceList mWeakRefList; // list of weak references
> > }
> > 
> > Make XPCOM object inherited from weak reference class and keep as the first
> > item of the weak reference list (downcast it to XPCOM object when requested).
> 
> how is  invalidation going to work? virtual function? (simple but not very
> nice)?

weak reference keeps a raw pointer to accessible object, when accessible goes away then raw point is nulled out. That's it if I don't miss anything.

> other than the downcasting for xpcom object we could store member pointers I
> suppose.

that'd be nice. Anyway let's introduce weak reference here and deal with XPCOM stuffs later.

(In reply to David Bolter [:davidb] from comment #3)
> I'm not sure I have the whole picture yet (or I've forgotten). Are we
> talking about have an nsWeakAccessible? (similar to the nsWeakFrame setup)

yes, about weak reference implementation for accessible objects (see bug summary? ;) ) Note, implementation is different from nsWeakFrame, our implementation is going to be performant so we can use weakrefs really wide.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > > # accessible events
> > 
> > as an alternative here what if when we shutdown an accessible we run through
> > the event queue and drop all but a hide event for the accessible on the
> > floor.  since by the time we say this accessibles state changed we'll also
> > say this accessible is dead.
> 
> hide events are not exception but anyway when you shutdown the accessible

but why can't they be?  if something happens and then we kill the object but don't tell anybody about the first thing until after we killed the object whose to say the first change ever took place?

> tree of m objects and has n events in the queue, that's it's n*m. Proposed
> weak reference approach is faster since it takes n.

wouldn't that be m * number of refs to each?
Also if in a sain world dropping an event on the floor would mean making the queue shorter and so coalescing would be faster.
What I mean is the complexity is a little more tricky than that.

> > > # single accessible iterator
> > 
> > do we want to weak ref this or just say it can only be used in place where
> > tree is not allowed to change (so probably just getting relations)?
> 
> I'm not sure what you mean by "where tree is not allowed to change".

same sort of thing as with IDRefsIterator now where you can't have one across DOM tree mutations or the weak ref to a content node might become invalid (thinking about what you did in bug 421242).

> Currently iterators are not a problem but new IA2 probably make us to expose
> this object externally. So we don't control its life cycle and don't want to
> keep defunct accessible alive.

ok, if that happens they yeah we definitely need this.  You'll have to fix IDRefsIteraotr as I say though.

> > > For that nsDocAccessible::mAccessibleCache can provide values:
> > > struct
> > > {
> > >   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle
> > 
> > It'd be nice if the parent did this, or just the doc itself.
> 
> parent probably no because I don't see anything bad in parent but its
> children recreation. Document yes, and that's what I proposed actually
> (since mAccessibleCache is managed by document).

yeah, not completely sure what I was thinking, other than it would be nice if doing this allowed us to stop refcounting accessibles, which is a little weird if the doc and the parent possily both want to own accessibles.

> > >   AccWeakReferenceList mWeakRefList; // list of weak references
> > > }
> > > 
> > > Make XPCOM object inherited from weak reference class and keep as the first
> > > item of the weak reference list (downcast it to XPCOM object when requested).
> > 
> > how is  invalidation going to work? virtual function? (simple but not very
> > nice)?
> 
> weak reference keeps a raw pointer to accessible object, when accessible
> goes away then raw point is nulled out. That's it if I don't miss anything.

yeah, that's fine if you just want weak refs, but I'm not sure if it works with the xpcom objects part of what you said.

> > other than the downcasting for xpcom object we could store member pointers I
> > suppose.
> 
> that'd be nice. Anyway let's introduce weak reference here and deal with
> XPCOM stuffs later.

Well, I'd prefer to avoid adding machinary when simpler things will do, but maybe we really do need this machinary.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > hide events are not exception but anyway when you shutdown the accessible
> 
> but why can't they be?  if something happens and then we kill the object but
> don't tell anybody about the first thing until after we killed the object
> whose to say the first change ever took place?

if hide event target is defunct then it means it was coalesced by other hide event.

> > tree of m objects and has n events in the queue, that's it's n*m. Proposed
> > weak reference approach is faster since it takes n.
> 
> wouldn't that be m * number of refs to each?

no, we have an event for root of subtree and therefore a weak ref for it, all children may not have weak refs.

> Also if in a sain world dropping an event on the floor would mean making the
> queue shorter and so coalescing would be faster.

while we keep them in array then it doesn't sound like a good approach

> What I mean is the complexity is a little more tricky than that.

Anyway I think I prefer to have unified approach rather than handling the same problem in different ways.

> > > > # single accessible iterator
> > > 
> > > do we want to weak ref this or just say it can only be used in place where
> > > tree is not allowed to change (so probably just getting relations)?
> > 
> > I'm not sure what you mean by "where tree is not allowed to change".
> 
> same sort of thing as with IDRefsIterator now where you can't have one
> across DOM tree mutations or the weak ref to a content node might become
> invalid (thinking about what you did in bug 421242).

well, I didn't think about correctness of iterators, I just wanted to make sure they don't keep an accessible alive when it should go away.

> 
> > Currently iterators are not a problem but new IA2 probably make us to expose
> > this object externally. So we don't control its life cycle and don't want to
> > keep defunct accessible alive.
> 
> ok, if that happens they yeah we definitely need this.  You'll have to fix
> IDRefsIteraotr as I say though.

I'm still not sure what you keep in mind. So, let's file a new bug if it's big enough or fix it here if it's small.

> > parent probably no because I don't see anything bad in parent but its
> > children recreation. Document yes, and that's what I proposed actually
> > (since mAccessibleCache is managed by document).
> 
> yeah, not completely sure what I was thinking, other than it would be nice
> if doing this allowed us to stop refcounting accessibles, which is a little
> weird if the doc and the parent possily both want to own accessibles.

yeah, that's what I want to do one day. As follow up of this bug we can switch to weak refs in parent-child relations of nsAccessible object, no strong refs, no cycle collection is needed. Next step would be get rid weak refs for those when we get tree update correct always.

> > > >   AccWeakReferenceList mWeakRefList; // list of weak references
> > > > }
> > > > 
> > > > Make XPCOM object inherited from weak reference class and keep as the first
> > > > item of the weak reference list (downcast it to XPCOM object when requested).
> > > 
> > > how is  invalidation going to work? virtual function? (simple but not very
> > > nice)?
> > 
> > weak reference keeps a raw pointer to accessible object, when accessible
> > goes away then raw point is nulled out. That's it if I don't miss anything.
> 
> yeah, that's fine if you just want weak refs, but I'm not sure if it works
> with the xpcom objects part of what you said.

so if you inherited xpcom object from weak ref then xpcom object has, say, mWeakRef to nsAccessbile. For each method it check if mWeakRef value is not null.

> > > other than the downcasting for xpcom object we could store member pointers I
> > > suppose.
> > 
> > that'd be nice. Anyway let's introduce weak reference here and deal with
> > XPCOM stuffs later.
> 
> Well, I'd prefer to avoid adding machinary when simpler things will do, but
> maybe we really do need this machinary.

do you concern XPCOM part implementation or general concern?

Comment 7

[Trevor Saunders (:tbsaunde)]

> > > tree of m objects and has n events in the queue, that's it's n*m. Proposed
> > > weak reference approach is faster since it takes n.
> > 
> > wouldn't that be m * number of refs to each?
> 
> no, we have an event for root of subtree and therefore a weak ref for it,
> all children may not have weak refs.

how are you not going to have a weak ref wherever you need to point to an accessible from an event?

> > Also if in a sain world dropping an event on the floor would mean making the
> > queue shorter and so coalescing would be faster.
> 
> while we keep them in array then it doesn't sound like a good approach

yeah, but as far as I can see the one big array approach isn't particularly sain.

> > What I mean is the complexity is a little more tricky than that.
> 
> Anyway I think I prefer to have unified approach rather than handling the
> same problem in different ways.

To me it would depend a lot on the complexity of the generic approach.  If we can simply take are of all the cases without a big generic thing why not on the other hand if we need 10 tricky things then maybe generic approach makes sense.

> > > > > # single accessible iterator
> > > > 
> > > > do we want to weak ref this or just say it can only be used in place where
> > > > tree is not allowed to change (so probably just getting relations)?
> > > 
> > > I'm not sure what you mean by "where tree is not allowed to change".
> > 
> > same sort of thing as with IDRefsIterator now where you can't have one
> > across DOM tree mutations or the weak ref to a content node might become
> > invalid (thinking about what you did in bug 421242).
> 
> well, I didn't think about correctness of iterators, I just wanted to make
> sure they don't keep an accessible alive when it should go away.

sure, but other than for ia2 you could just say that accessible must not go defunct while iterator points at it, and change the RefPtr in SingleAccIterator to a raw pointer and call it a day.

> > 
> > > Currently iterators are not a problem but new IA2 probably make us to expose
> > > this object externally. So we don't control its life cycle and don't want to
> > > keep defunct accessible alive.
> > 
> > ok, if that happens they yeah we definitely need this.  You'll have to fix
> > IDRefsIteraotr as I say though.
> 
> I'm still not sure what you keep in mind. So, let's file a new bug if it's
> big enough or fix it here if it's small.

its completely different issue, but IDRefsIterator has member

nsIContent* mContent;

Then if I can hold an IDRefsIterator accross DOM mutation we have a problem, and either need to be very clever, or make that reference strong.

> > > parent probably no because I don't see anything bad in parent but its
> > > children recreation. Document yes, and that's what I proposed actually
> > > (since mAccessibleCache is managed by document).
> > 
> > yeah, not completely sure what I was thinking, other than it would be nice
> > if doing this allowed us to stop refcounting accessibles, which is a little
> > weird if the doc and the parent possily both want to own accessibles.
> 
> yeah, that's what I want to do one day. As follow up of this bug we can
> switch to weak refs in parent-child relations of nsAccessible object, no
> strong refs, no cycle collection is needed. Next step would be get rid weak
> refs for those when we get tree update correct always.

I'm not sure what allways means, I suspect there will always be atleast one bug somewhere.

my thoughts had been to just go ahead make the parent pointer raw, and have the document stop holding owning references to accessibles other than direct kids, using the requirement that it must be notified when a accessible is removed from the doc.

> > > > other than the downcasting for xpcom object we could store member pointers I
> > > > suppose.
> > > 
> > > that'd be nice. Anyway let's introduce weak reference here and deal with
> > > XPCOM stuffs later.
> > 
> > Well, I'd prefer to avoid adding machinary when simpler things will do, but
> > maybe we really do need this machinary.
> 
> do you concern XPCOM part implementation or general concern?

its a general thing.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > > tree of m objects and has n events in the queue, that's it's n*m. Proposed
> > > > weak reference approach is faster since it takes n.
> > > 
> > > wouldn't that be m * number of refs to each?
> > 
> > no, we have an event for root of subtree and therefore a weak ref for it,
> > all children may not have weak refs.
> 
> how are you not going to have a weak ref wherever you need to point to an
> accessible from an event?

I think you misunderstood me. Let's you have a tree of m elements you're going to destroy, let's you have events queue of n events so you should run through events queue for each element from tree tree, at least until we do perfect coalition.

> > > What I mean is the complexity is a little more tricky than that.
> > 
> > Anyway I think I prefer to have unified approach rather than handling the
> > same problem in different ways.
> 
> To me it would depend a lot on the complexity of the generic approach.  If
> we can simply take are of all the cases without a big generic thing why not
> on the other hand if we need 10 tricky things then maybe generic approach
> makes sense.

i'm up for general approach because this approach is fast and let us to not thing about all different cases in invalidation.

> sure, but other than for ia2 you could just say that accessible must not go
> defunct while iterator points at it, and change the RefPtr in
> SingleAccIterator to a raw pointer and call it a day.

yes, other than ia2 case

> its completely different issue, but IDRefsIterator has member
> 
> nsIContent* mContent;
> 
> Then if I can hold an IDRefsIterator accross DOM mutation we have a problem,
> and either need to be very clever, or make that reference strong.

I see, we need to keep this in mind if IA2 is changed

> my thoughts had been to just go ahead make the parent pointer raw, and have
> the document stop holding owning references to accessibles other than direct
> kids, using the requirement that it must be notified when a accessible is
> removed from the doc.

ok, this approach makes hard to keep the case when parent is recreated but its children not. I think making a document to manage the lifecycle of accessible object is correct, at least it's responsible for tree invalidation what means we keep the logic in the same place.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > > > tree of m objects and has n events in the queue, that's it's n*m. Proposed
> > > > > weak reference approach is faster since it takes n.
> > > > 
> > > > wouldn't that be m * number of refs to each?
> > > 
> > > no, we have an event for root of subtree and therefore a weak ref for it,
> > > all children may not have weak refs.
> > 
> > how are you not going to have a weak ref wherever you need to point to an
> > accessible from an event?
> 
> I think you misunderstood me. Let's you have a tree of m elements you're
> going to destroy, let's you have events queue of n events so you should run
> through events queue for each element from tree tree, at least until we do
> perfect coalition.

ok, I think we may have talked passed each other some.  What you say is true, but I'm not sure I agree its fair to say weak refs is linear in number of accessibles.

lets say you have n accessibles to shutdown which have x1, x2, .... xn weak refs each in the event queue.  I'd say the correct run time of this is \sigma_{i=1}^n x_i

> > > > What I mean is the complexity is a little more tricky than that.
> > > 
> > > Anyway I think I prefer to have unified approach rather than handling the
> > > same problem in different ways.
> > 
> > To me it would depend a lot on the complexity of the generic approach.  If
> > we can simply take are of all the cases without a big generic thing why not
> > on the other hand if we need 10 tricky things then maybe generic approach
> > makes sense.
> 
> i'm up for general approach because this approach is fast and let us to not
> thing about all different cases in invalidation.

I'm honestly not sure what I think, if we had something that clearerly neededweak refs now I'd say absolutely, but for now I'm sort of in middle.

> > sure, but other than for ia2 you could just say that accessible must not go
> > defunct while iterator points at it, and change the RefPtr in
> > SingleAccIterator to a raw pointer and call it a day.
> 
> yes, other than ia2 case

which is a issue we don't handle currently.  So do we
* fix the other obviously places this possible ia2 issue could come up
* ignore this possible future issue and just keep raw pointer and say it must need be held across tree mutation
* use weak ref here and leave other case is so some iterators work in ia2 case but not others, and we take small perf hit to have some iterators possibly already working when ia2 actually does this.

I'd probably say just use raw pointer and restrict usage for now since it shouldn't be hard to make this work later if ia2 does start requiring us to hold one across tree mutation.

> > my thoughts had been to just go ahead make the parent pointer raw, and have
> > the document stop holding owning references to accessibles other than direct
> > kids, using the requirement that it must be notified when a accessible is
> > removed from the doc.
> 
> ok, this approach makes hard to keep the case when parent is recreated but
> its children not. I think making a document to manage the lifecycle of
> accessible object is correct, at least it's responsible for tree
> invalidation what means we keep the logic in the same place.

yeah, I guess this part is fine by me if it allows us to make document sole owner of the accessible.  I'd been thinking of having the parent be the owner since afaik that's more standard, but the doc does seem to make more sense.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> (In reply to alexander surkov from comment #0)
> 
> > # XPCOM objects
> 
> Approach for XPCOM objects is to keep own hash to get XPCOM object by given
> accessible object so it can be sort of own weak reference implementation,
> i.e. when accessible goes away then it's removed from hash and accessible
> pointer in XPCOM object is null out.
> 
> Having said this approach can be used as a base for common weak reference
> implementation, i.e. use hash to get weak reference list by accessible
> object.
> 
> For that nsDocAccessible::mAccessibleCache can provide > struct
> {
>   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle

that seems kind of wasteful, if we really want to use a ref pointer it seems like sticking it in the mNodeToAccessibleMap would be better, or we could just manually AddRef / Release the accessible when we bind / unbind it.

>   AccWeakReferenceList mWeakRefList; // list of weak references

can't that just be nsTArray<nsAccessible**> although if you like a typedef ok.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > I think you misunderstood me. Let's you have a tree of m elements you're
> > going to destroy, let's you have events queue of n events so you should run
> > through events queue for each element from tree tree, at least until we do
> > perfect coalition.
> 
> ok, I think we may have talked passed each other some.  What you say is
> true, but I'm not sure I agree its fair to say weak refs is linear in number
> of accessibles.
> 
> lets say you have n accessibles to shutdown which have x1, x2, .... xn weak
> refs each in the event queue.  I'd say the correct run time of this is
> \sigma_{i=1}^n x_i

anyway, if weakrefs are used in events only then it's lesser than m*n. if weakrefs are used outside events then it could be > m*n but in this case we need to do invalidation of these other places.

> > i'm up for general approach because this approach is fast and let us to not
> > thing about all different cases in invalidation.
> 
> I'm honestly not sure what I think, if we had something that clearerly
> neededweak refs now I'd say absolutely, but for now I'm sort of in middle.

other point of weak refs was keep a weak ref to internal object in MSAA objects.

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > For that nsDocAccessible::mAccessibleCache can provide > struct
> > {
> >   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle
> 
> that seems kind of wasteful, if we really want to use a ref pointer it seems
> like sticking it in the mNodeToAccessibleMap would be better, or we could
> just manually AddRef / Release the accessible when we bind / unbind it.

not sure I follow, why manual AddRef/Release are prefered and why mNodeToAccessibleMap is better?

> >   AccWeakReferenceList mWeakRefList; // list of weak references
> 
> can't that just be nsTArray<nsAccessible**> although if you like a typedef
> ok.

sure, it was an illustration :)

Comment 12

[Trevor Saunders (:tbsaunde)]

> > > i'm up for general approach because this approach is fast and let us to not
> > > thing about all different cases in invalidation.
> > 
> > I'm honestly not sure what I think, if we had something that clearerly
> > neededweak refs now I'd say absolutely, but for now I'm sort of in middle.
> 
> other point of weak refs was keep a weak ref to internal object in MSAA
> objects.

yeah, but we don't really need this to do that easily, we could just do the same thing we do in atk nsAccessibleWrap and implement Shutdown() to kill the pointer in the wrapper object.  That doesn't really work if we get rid of nsAccessibleWrap, but if we do that we need to more generally rethink how wrapper object tree is updated etc.

> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > > For that nsDocAccessible::mAccessibleCache can provide > struct
> > > {
> > >   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle
> > 
> > that seems kind of wasteful, if we really want to use a ref pointer it seems
> > like sticking it in the mNodeToAccessibleMap would be better, or we could
> > just manually AddRef / Release the accessible when we bind / unbind it.
> 
> not sure I follow, why manual AddRef/Release are prefered and why
> mNodeToAccessibleMap is better?

I was mostly trying to not need more hash tables, but I'm not sure I actually think they are, sort of just putting out ideas in the hope some of them will end up making sense or giving someone else a good idea.

> > >   AccWeakReferenceList mWeakRefList; // list of weak references
> > 
> > can't that just be nsTArray<nsAccessible**> although if you like a typedef
> > ok.
> 
> sure, it was an illustration :)

ok, sounds good.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> > other point of weak refs was keep a weak ref to internal object in MSAA
> > objects.
> 
> yeah, but we don't really need this to do that easily, we could just do the
> same thing we do in atk nsAccessibleWrap and implement Shutdown() to kill
> the pointer in the wrapper object.  That doesn't really work if we get rid
> of nsAccessibleWrap, but if we do that we need to more generally rethink how
> wrapper object tree is updated etc.

that means nsAccessibleWrap should keep a pointer to MSAA object (if we start to use tear offs for MSAA then things get more tricky)

It seems to be we battle over having a general approach vs handling each case separately.

> > (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > > > For that nsDocAccessible::mAccessibleCache can provide > struct
> > > > {
> > > >   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle
> > > 
> > > that seems kind of wasteful, if we really want to use a ref pointer it seems
> > > like sticking it in the mNodeToAccessibleMap would be better, or we could
> > > just manually AddRef / Release the accessible when we bind / unbind it.
> > 
> > not sure I follow, why manual AddRef/Release are prefered and why
> > mNodeToAccessibleMap is better?
> 
> I was mostly trying to not need more hash tables, but I'm not sure I
> actually think they are, sort of just putting out ideas in the hope some of
> them will end up making sense or giving someone else a good idea.

we just reuse existing hashtable

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> 
> > > other point of weak refs was keep a weak ref to internal object in MSAA
> > > objects.
> > 
> > yeah, but we don't really need this to do that easily, we could just do the
> > same thing we do in atk nsAccessibleWrap and implement Shutdown() to kill
> > the pointer in the wrapper object.  That doesn't really work if we get rid
> > of nsAccessibleWrap, but if we do that we need to more generally rethink how
> > wrapper object tree is updated etc.
> 
> that means nsAccessibleWrap should keep a pointer to MSAA object (if we
> start to use tear offs for MSAA then things get more tricky)

I'm not sure I see what you mean here.  We're just going to have one platform object that implements all of the msaa / ia2 methods for that object no?

> It seems to be we battle over having a general approach vs handling each
> case separately.

yeah, and the number of case there will be.

> > > (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > > > > For that nsDocAccessible::mAccessibleCache can provide > struct
> > > > > {
> > > > >   nsRefPtr<nsAccessible> mAccessible; // manage accessible lifecycle
> > > > 
> > > > that seems kind of wasteful, if we really want to use a ref pointer it seems
> > > > like sticking it in the mNodeToAccessibleMap would be better, or we could
> > > > just manually AddRef / Release the accessible when we bind / unbind it.
> > > 
> > > not sure I follow, why manual AddRef/Release are prefered and why
> > > mNodeToAccessibleMap is better?
> > 
> > I was mostly trying to not need more hash tables, but I'm not sure I
> > actually think they are, sort of just putting out ideas in the hope some of
> > them will end up making sense or giving someone else a good idea.
> 
> we just reuse existing hashtable

yeah, but it would be nice if we didn't have that hash table, but I guess if we have it why not use it.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> > that means nsAccessibleWrap should keep a pointer to MSAA object (if we
> > start to use tear offs for MSAA then things get more tricky)
> 
> I'm not sure I see what you mean here.  We're just going to have one
> platform object that implements all of the msaa / ia2 methods for that
> object no?

ok, I thought about ISimpleDOM interfaces, but we could not use tear offs for them

> > It seems to be we battle over having a general approach vs handling each
> > case separately.
> 
> yeah, and the number of case there will be.

so candidates are:
1) XPCOM
2) MSAA (and other platforms)
3) events
4) relations if IA2 1.3 will be released ever
5) possibly parent/child relations (replace strong ref on weak ref)

we could handle all of them via weak refs or maintain invalidation logic for each of them. I like to keep things the same way if it doesn't hit us too much.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch

plain and easy, it makes us dependent on caches but benefits should prevail over that

Comment 17

[alexander :surkov (:asurkov)]

Trev, I thought you commented about this bug but I didn't find where. So what do you think?

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #17)
> Trev, I thought you commented about this bug but I didn't find where. So
> what do you think?

doesn't the approach suffer from the A B A problem? that is to say if I first have node with accessible at address x remove it kill the accessible then create a new node at X and it gets an accessible  any weak refs that pointed at the first accessible and are still around again refer to an accessible just a different one!

Comment 19

[alexander :surkov (:asurkov)]

it is, ideas to workaround it?

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #19)
> it is, ideas to workaround it?

use hash map from accessible to weak refs? or just not solve the problem?  (what do we need it for?)

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #20)
> (In reply to alexander :surkov from comment #19)
> > it is, ideas to workaround it?
> 
> use hash map from accessible to weak refs?

4 bytes per week ref vs keeping an accessible object in memory longer than we need. Probably a win if somebody keeps them.

> or just not solve the problem? 
> (what do we need it for?)

It's tempting but imagine AT stored ISimpleDOMNode for menu accessible, it was replaced on different accessible at the same memory address but AT still operates on it like it was a menu. It could be a problem.

Comment 22

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #21)
> (In reply to Trevor Saunders (:tbsaunde) from comment #20)
> > (In reply to alexander :surkov from comment #19)
> > > it is, ideas to workaround it?
> > 
> > use hash map from accessible to weak refs?
> 
> 4 bytes per week ref vs keeping an accessible object in memory longer than
> we need. Probably a win if somebody keeps them.

maybe, but its heavy weight and I'm not yet convinced we need it.

> > or just not solve the problem? 
> > (what do we need it for?)
> 
> It's tempting but imagine AT stored ISimpleDOMNode for menu accessible, it
> was replaced on different accessible at the same memory address but AT still
> operates on it like it was a menu. It could be a problem.

oh, yeah, I don't think not solvingthat problem is acceptable, I meant not supporting weak refs.

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 742208 [details] [diff] [review]
patch

ok, leaving it for now

Comment 24

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.