227489 - nsIWidget::GetParent and GetChildren should not return raw addrefed pointers

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Boris Zbarsky [:bzbarsky]]

GetParent() and GetChildren() should return already_AddRefed (or GetParent()
should maybe just return a weak pointer).

Comment 1

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

*** Bug 251527 has been marked as a duplicate of this bug. ***

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that GetChildren() is gone.  All that's left is GetParent().

Comment 3

[Bastiaan Jacques]

So..

Instead of:

nsIWidget* nsWidget::GetParent(void)
{
  nsIWidget *ret;
  ret = mParent;
  NS_IF_ADDREF(ret);
  return ret;
}

do we want:

already_AddRefed<nsIWidget> nsWidget::GetParent()
{
  return do_QueryReferent(mParent);
}

, assuming mParent is an nsWeakPtr?

Comment 4

[Boris Zbarsky [:bzbarsky]]

No, we want:

nsIWidget* GetParent() const { return mParent; }

(mParent is not an nsWeakPtr).

Then we need to find all callers of this method and audit them to make sure that
nothing bad will happen.  This is the part that's pretty nontrivial to do.  :(

Comment 5

[Bastiaan Jacques]

(In reply to comment #4)
> Then we need to find all callers of this method and audit them to make sure that
> nothing bad will happen.  This is the part that's pretty nontrivial to do.  :(

Okay. Should the callers have a strong reference to the returned pointer while
they're using it?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Depends on the caller.  All the ones that can get away with holding a weak ref
(i.e. all the ones that don't do anything that could destroy the widget) should
do so.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Just fix GetParent()

I looked at all GetParent() callers in the tree, and things look ok.  Note that this fixes an accessibility leak...

Comment 8

[Boris Zbarsky [:bzbarsky]]

roc, could you possibly land that for me?  I won't have a chance to watch the tree for at least 5 days, maybe more, and this sort of thing rots easily.  :(

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll land it for you.

Comment 10

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

(In reply to comment #7)
> Note that this fixes an accessibility leak...

Where?

Comment 11

[Boris Zbarsky [:bzbarsky]]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsDocAccessible.cpp&rev=1.82&mark=209,217-220#207

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed.