445910 - use XPCOM cycle collector in accessibility classes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

use XPCOM cycle collector in accessibility classes

Comment 1

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 2

[Aaron Leventhal]

Does this fix our leak bugs, like bug 434491, bug 420068 or bug 443675?

Comment 3

[alexander :surkov (:asurkov)]

Shortly what I wanted to do: we have an accessible tree, every accessible
object of the tree holds pointers to parent, next sibling and first child
accessible objects. Currently parent is nsCOMPtr, nextSibling and firstChild
are raw pointers. I wanted to make to use nsCOMPtr for nextSibling and
fisrtChild as well. Therefore I thought XPCOM cycle collector should helps
here. But when I use nsCOMPtr for nextSibling and firstChild pointers and add
them into traverse and unlink marcros then firefox hangs and then crashes. If I
don't put nextSibling and firstChild into traverse and unlink macros then it
works. Please look at the patch. Do you have any idea what's wrong? Does cycle
collector work with objects that holds nsCOMPtr pointers for each other?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Does this fix our leak bugs, like bug 434491, bug 420068 or bug 443675?
> 

I think it fix nothing now :). Because we should turn raw pointers into nsCOMPtr. I'm not sure whether it helps with memory leaks but it should kill our crashes.

Comment 5

[alexander :surkov (:asurkov)]

Actually I can't find any documentation about cycle collector (excepting probably http://developer.mozilla.org/en/docs/Interfacing_with_the_XPCOM_cycle_collector). And I can't find in mozilla sources our case.

Comment 6

[alexander :surkov (:asurkov)]

Possibly traversing should move forward only, for example, down through the children? And when I added traversing through parent and children then I locked the circle?

Comment 7

[alexander :surkov (:asurkov)]

Attachment: wip (traverse next sibling and first child only, doesn't work)

traversing to next sibling and firsrt child doesn't help (firefox hangs and crashed then)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 330210 [details] [diff] [review]
wip (traverse next sibling and first child only, doesn't work)

Random comments...

>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsAccessNode)
>+  NS_INTERFACE_MAP_ENTRY(nsIAccessNode)
>+  NS_INTERFACE_MAP_ENTRY(nsPIAccessNode)
Should you add NS_INTERFACE_MAP_ENTRY(nsISupports)
or maybe
NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessNode)
?

>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsAccessible)
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsAccessible, nsAccessNode)
>+//  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mParent)
Why commented out?

Who is keeping nsAccessible alive and when is nsAccessible released?

Comment 9

[alexander :surkov (:asurkov)]

Attachment: wip3

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> >+NS_IMPL_CYCLE_COLLECTION_CLASS(nsAccessible)
> >+
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsAccessible, nsAccessNode)
> >+//  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mParent)
> Why commented out?

I thought that may be a problem (traversing up and down). I fixed that in wip3 patch

> Who is keeping nsAccessible alive and when is nsAccessible released?
> 

1) they keep each other by mParent, mNextSibling, mFirstChild
2) nsAccessNodeHashtable nsDocAccessible::mAccessNodeCache - cashe of all accessibles that belongs to the document
3) nsCOMArray<nsIAccessibleEvent> nsDocAccessible::mEventsToFire - technically accessibles object may be hold by nsIAccessibleEvent but it shouldn't happen
4) for example, in windows accessible objects may be used by screen readers via COM

But why does it hang even if I don't process here all cases? I thought it may lead to memory leaks only.

Comment 11

[Peter Van der Beken [:peterv]]

There's plenty of ways to crash, for example your code might not be able to deal with the null members after unlinking. Another example, how are you dealing with mNextSibling == DEAD_END_ACCESSIBLE? I suggest you first get some more details about the crashes.

Comment 12

[alexander :surkov (:asurkov)]

Thanks, Pete. You're right the problem is in DEAD_END_ACCESSIBLE

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch

Marco, could you try the patch?

Comment 14

[Marco Zehe (:MarcoZ)]

So far, except for one thing, I haven't seen anything bad. No crashes or anything like that.

The one thing I did observe was this. With JAWS running:
1. Visit http://www.mozilla.org.
2. Press CTRL+L to go to Location bar.
3. Type in about:blank and hit enter.

There was an instance where JAWS would not, for the life of it, let go of its previous document. So even when Firefox was showing a blank document, JAWS's virtual buffer still showed the Mozilla Foundation homepage.

Later, I could not reproduce this, and I also could not reproduce using NVDA.

Regarding bug 429030: There is a change in behaviour: I no longer crash Firefox. Instead, when trying to navigate to the next sibling from the Page accessible, I now hear an error tone from NVDA. So, NVDA notices that there's something wrong, and Firefox does not crash.
Without this patch, or with the current nightly, this error tone would not be heard, but Firefox would crash.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch2

since nsDocAccessible caches all children from its subtree then there is a direct child which mParent points to  nsDocAccessible. Therefore I added cycle collection to it.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Does this fix our leak bugs, like bug 434491

can't reproduce on trunk

, bug 420068 

doesn't fix :(

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> (In reply to comment #2)
> > Does this fix our leak bugs, like bug 434491
> 
> can't reproduce on trunk

there is leaks of nsAccessNode, patch doesn't help

Comment 18

[Marco Zehe (:MarcoZ)]

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

So far, no negative impact noticed. Behaviour with NVDA is the same as with previous patch, which means "error sound", but no crash. Also, the bogus document hasn't reappeared yet.

Comment 19

[Marco Zehe (:MarcoZ)]

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

r=me. I have been running with this patch for 2 days straight and didn't find any regressions. No crashes or other unexpected behavior occurred.

Comment 20

[[e-mail]]

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

>+NS_IMPL_CYCLE_COLLECTION_0(nsAccessNode)
Can anyone actually create a raw access node?
(Note: I don't actually see the point of NS_IMPL_CYCLE_COLLECTION_0 since an object that owns no collectable objects can't be part of a cycle.)

>+NS_IMPL_CYCLE_COLLECTION_CLASS(nsAccessible)
>+
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsAccessible, nsAccessNode)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mParent)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFirstChild)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mNextSibling)
>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
>+
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsAccessible, nsAccessNode)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mParent)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mFirstChild)
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mNextSibling)
>+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
Why not use NS_IMPL_CYCLE_COLLECTION_3?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> (From update of attachment 330244 [details] [diff] [review])
> >+NS_IMPL_CYCLE_COLLECTION_0(nsAccessNode)
> Can anyone actually create a raw access node?
> (Note: I don't actually see the point of NS_IMPL_CYCLE_COLLECTION_0 since an
> object that owns no collectable objects can't be part of a cycle.)

Yes, but I didn't found macros for this case. Object with cycle collections have specific addref/release method implementation, here I need that and plus redirect them to base class.

> >+NS_IMPL_CYCLE_COLLECTION_CLASS(nsAccessible)
> >+
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsAccessible, nsAccessNode)
> >+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mParent)
> >+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFirstChild)
> >+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mNextSibling)
> >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> >+
> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsAccessible, nsAccessNode)
> >+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mParent)
> >+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mFirstChild)
> >+  NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMPTR(mNextSibling)
> >+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> Why not use NS_IMPL_CYCLE_COLLECTION_3?
> 

because there is no NS_IMPL_CYCLE_COLLECTION_ INHERITED_3?

Comment 22

[[e-mail]]

Sorry I wasn't being clear, but if the nsAccessNode class is never itself created as an object then it doesn't need to participate in cycle collection and nsAccessible doesn't need to implement inherited cycle collection.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> Sorry I wasn't being clear, but if the nsAccessNode class is never itself
> created as an object then it doesn't need to participate in cycle collection
> and nsAccessible doesn't need to implement inherited cycle collection.
> 

There are cases when nsAccessNode is created (via COM's ISimple* interfaces when DOM node isn't accessible in the usual sense). Though I didn't catch you what the difference does it play?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (In reply to comment #22)
> > Sorry I wasn't being clear, but if the nsAccessNode class is never itself
> > created as an object then it doesn't need to participate in cycle collection
> > and nsAccessible doesn't need to implement inherited cycle collection.
> > 
> 
> There are cases when nsAccessNode is created (via COM's ISimple* interfaces
> when DOM node isn't accessible in the usual sense). Though I didn't catch you
> what the difference does it play?
> 

Do you mean in this case we wouldn't call AddRef/Release of nsAccessNode? And even we wouldn't implement those methods, right?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

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

>-class nsAccessNode: public nsIAccessNode, public nsPIAccessNode
>+class nsAccessNode: public nsIAccessNode,
>+                    public nsPIAccessNode
> {
>   public: // construction, destruction
>     nsAccessNode(nsIDOMNode *, nsIWeakReference* aShell);
>     virtual ~nsAccessNode();
> 
>-    NS_DECL_ISUPPORTS
>+ //   NS_DECL_ISUPPORTS
Remove, don't just comment out.

> NS_IMETHODIMP nsAccessible::InvalidateChildren()
> {
>   // Document has transformed, reset our invalid children and child count
> 
>   // Reset the sibling pointers, they will be set up again the next time
>   // CacheChildren() is called.
>   // Note: we don't want to start creating accessibles at this point,
>   // so don't use GetNextSibling() here. (bug 387252)
>-  nsAccessible* child = static_cast<nsAccessible*>(mFirstChild);
>+  nsAccessible* child = static_cast<nsAccessible*>(mFirstChild.get());
>+//  nsAccessible* child = static_cast<nsAccessible*>(mFirstChild);
Same here.

Comment 26

[[e-mail]]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 330244 [details] [diff] [review] [details])
> > >+NS_IMPL_CYCLE_COLLECTION_0(nsAccessNode)
> > Can anyone actually create a raw access node?
> > (Note: I don't actually see the point of NS_IMPL_CYCLE_COLLECTION_0 since an
> > object that owns no collectable objects can't be part of a cycle.)
peterv says that you need this so that subclasses can be cycle collected.

> > Why not use NS_IMPL_CYCLE_COLLECTION_3?
> because there is no NS_IMPL_CYCLE_COLLECTION_ INHERITED_3?
Sorry, I hadn't realised that NS_IMPL_CYCLE_COLLECTION_3_AMBIGUOUS had been surreptitiously removed by bug 368549.

Comment 27

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #26)
> (In reply to comment #21)
> > because there is no NS_IMPL_CYCLE_COLLECTION_ INHERITED_3?
> Sorry, I hadn't realised that NS_IMPL_CYCLE_COLLECTION_3_AMBIGUOUS had been
> surreptitiously removed by bug 368549.

*_AMBIGUOUS and *_INHERITED macros are very different.  _AMBIGUOUS is for classes that that can't convert to/from nsISupports without an intermediate step (to clarify the ambiguity) in between, because the class inherits from nsISupports in multiple ways.  The extra class given to a *_AMBIGUOUS macro is usually an interface.  *_INHERITED macros are to forward to the base class implementation.  The extra class given to a *_INHERITED macro is always a concrete class.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #26)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 330244 [details] [diff] [review] [details] [details])
> > > >+NS_IMPL_CYCLE_COLLECTION_0(nsAccessNode)
> > > Can anyone actually create a raw access node?
> > > (Note: I don't actually see the point of NS_IMPL_CYCLE_COLLECTION_0 since an
> > > object that owns no collectable objects can't be part of a cycle.)
> peterv says that you need this so that subclasses can be cycle collected.

So, I don't need to change anything?

Comment 29

[[e-mail]]

(In reply to comment #28)
> So, I don't need to change anything?
No.

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #21)
> > > because there is no NS_IMPL_CYCLE_COLLECTION_ INHERITED_3?
> > Sorry, I hadn't realised that NS_IMPL_CYCLE_COLLECTION_3_AMBIGUOUS had been
> > surreptitiously removed by bug 368549.
> *_AMBIGUOUS and *_INHERITED macros are very different.
Ah yes, I should have realised when the macros used _base instead of Super.

Comment 30

[alexander :surkov (:asurkov)]

Neil, any other comments?

Comment 31

[[e-mail]]

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

Sorry, I thought I'd done this one :-(

Comment 32

[Aaron Leventhal]

Will we be warned when the cycle ends up getting used? Because it shouldn't have to be if we did things correctly.

Comment 33

[Aaron Leventhal]

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

I'm basing my r= on Neil's sr= on the actual code. I have no issues with this going into the module.

Comment 34

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/f97926604480