368760 - NotifySelectionListeners might lead to self destruction

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[Mats Palmgren (:mats)]

See the stack from bug 368697: attachment 253357 [details]

As roc pointed out it might also lead to destroying 'this'.

I guess the obvious solution is to fire off the NotifySelectionListeners
call from an event, but is that enough I wonder?
Perhaps for the internal calls leading up to NotifySelectionListeners, but
what about external callers... e.g. editor code - can the editor be destroyed
as well?  The whole thing seems rather fragile, it only takes one call
to FlushPendingNotifications() and we're back to square one...
I'm not sure if adding "kungFuDeathGrip(this)" all over the place is
desired either...

Comment 1

[Mats Palmgren (:mats)]

Internal paths leading up to NotifySelectionListeners()

NotifySelectionListeners
  Collapse
    MoveCaret
      CharacterMove
      WordMove
      WordExtendForDelete
      LineMove
      IntraLineMove
    AdjustForMaintainedSelection
      HandleClick
        CommonPageMove
        HandleDrag
          nsAutoScrollTimer::Notify
    TakeFocus
      VisualSequence
      SelectToEdge
      SelectLines
      VisualSelectFrames
      HandleClick
      SelectAll
    HandleTableSelection
    DeleteFromDocument
    CollapseToStart
    CollapseToEnd
    SelectAllChildren
  Extend
  RemoveRange
    SelectBlockOfCells
  AddRange
    CreateAndAddRange
      SelectCellElement
        SelectRowOrColumn
  RemoveAllRanges
    ClearNormalSelection
  EndBatchChanges
  SetMouseDownState

Comment 2

[Boris Zbarsky [:bzbarsky]]

So maybe we should actually enforce XPCOM stuff.  Like "you must hold a pointer to the callee across the call no matter what".  If we can't because we have non-XPCOM classes on the stack, then the first caller into non-XPCOM code should flish pending notifications... and no one under that call should then be modifying the DOM.

Or something.  I agree that just throwing around kung-fu-death-grips is fragile.

Comment 3

[Mats Palmgren (:mats)]

FWIW, I think there is a potential crash on using the "tableLayoutObject"
(a table frame) after RemoveRange() have been called in SelectBlockOfCells().
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.277&root=/cvsroot&mark=3228,3249,3263,3284#3226

Comment 4

[Mats Palmgren (:mats)]

Attachment: Patch rev. 1

Do NotifySelectionListeners() off an event.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm sorry, bug there's no way I'm competent to review this code.  Someone who has some familiarity with it should look.... perhaps one of the editor peers?  Especially Simon?

Comment 6

[Simon Fraser [no longer active]]

This changes affects the call timing of lots of other things:
http://lxr.mozilla.org/mozilla/ident?i=NotifySelectionChanged
including onselect events, caret drawing, command updating etc. Please check all of those to make sure that there are no unwanted side effects.

Comment 7

[Daniel Veditz [:dveditz]]

This is old code so potentially affects the 1.8 branch as well, right?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 254524 [details] [diff] [review]
Patch rev. 1

Please rerequest sr once this has review...

Comment 9

[Damon Sicore (:damons)]

Mats, can you please address Comment #6 ?

Comment 10

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

Mats, what's the story here?

Comment 11

[Damon Sicore (:damons)]

Seconding Roc's request for followup.  :)

Comment 12

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

This is a scary area with possible regressions and stuff, so we really need it addressed for beta2.

Comment 13

[David Baron :dbaron:]

Smaug, any chance you'd be able to pick this one up and figure out what to do here?

Comment 14

[Olli Pettay [:smaug]]

Attachment: alternative approach

This patch forces strong ownership of nsFrameSelection in most cases and changes
nsTypedSelection<->nsFrameSelection ownership so that if nsFrameSelection
has created the nsTypedSelection object, nsFrameSelection is guaranteed to outlive nsTypedSelection.
So if nsTypedSelection objects and nsFrameSelection objects are handled using
strong pointers, things should be safer.
Comments? Mats, what do you think about this?

Comment 15

[Mats Palmgren (:mats)]

Comment on attachment 289462 [details] [diff] [review]
alternative approach

I think this is a better approach; it's more robust and less risky.

> This patch forces strong ownership of nsFrameSelection in most cases

IMO, we should make that an explicit requirement: consumers of
nsFrameSelection MUST hold a strong ref. - if we don't require that
then we need to add kungFuDeathGrip(this) in most methods listed in
comment 1.

We also need to something about the current use of |mShell|.
AFAICT, it's a non-addref'ed plain pointer:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrameSelection.h&rev=1.19&root=/cvsroot&mark=639#639
We need to handle the case were the nsFrameSelection out-lives the shell -
which will be more likely now that we extend the life of the
nsFrameSelection with strong refs.  Making |mShell| a nsWeakPtr is
probably the way to go.

Also, when dealing with frames, null-checking |mShell| isn't enough.
For example:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.296&root=/cvsroot&mark=3314,3324#3302
the |tableLayoutObject| frame can be destroyed by line 3324.
Holding a strong ref on |this| or even |mShell| doesn't help, we must
use nsWeakFrame or lookup the frame again.
Also watch out for other non-ref-counted things, like views:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.296&root=/cvsroot&mark=563#521
|captureView| might have been destroyed by HandleDrag().


A few comments on the patch:

In nsFrameSelection::MoveCaret and NotifySelectionListeners: 
we don't need the |kungFuDeathGrip(this)|

-      mDomSelections[i]->DetachFromPresentation();
-      NS_RELEASE(mDomSelections[i]);
+      delete mDomSelections[i]; // Calls DetachFromPresentation();

This was the last outside caller - we can remove DetachFromPresentation()
and move its contents to the destructor.

Maybe I'm missing something but I didn't quite get the point of moving
nsTextInputSelectionImpl::mFrameSelection to a super-class and then
using stack based strong refs... consumers of nsTextInputSelectionImpl
(read nsISelectionController) are already required to have a strong ref,
so mFrameSelection can't be destroyed...
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsTextControlFrame.cpp&rev=3.271&root=/cvsroot&mark=884,886#869
could crash otherwise (if |this| was destroyed).
We can remove the mFrameSelection null-checks though.

If we don't want that requirement on nsISelectionController consumers,
then I do see the point, but then we also need kungFuDeathGrip(this)
in a few nsTextInputSelectionImpl methods.

Comment 16

[Mats Palmgren (:mats)]

Comment on attachment 289462 [details] [diff] [review]
alternative approach

BTW, this crashes on Linux with an infinite stack of
nsTypedSelection::AddRef() calls.
Casting |this| to a super-class in NS_IMPL_ADDREF_USING_AGGREGATOR doesn't
help, it will still use the vtable for the AddRef() call.  We must use an
explicit super::AddRef() to avoid that.  It's probably clearer to just
implement AddRef/Release explicitly here instead of using the macros.

Comment 17

[Olli Pettay [:smaug]]

(In reply to comment #15)
> (From update of attachment 289462 [details] [diff] [review])
> I think this is a better approach; it's more robust and less risky.
> 
> > This patch forces strong ownership of nsFrameSelection in most cases
> 
> IMO, we should make that an explicit requirement: consumers of
> nsFrameSelection MUST hold a strong ref.
That is why methods return already_addrefed.

 - if we don't require that
> then we need to add kungFuDeathGrip(this) in most methods listed in
> comment 1.
Well, keeping nsTypedSelection alive is enough, if the Addref/release trick works. Ofc the users of nsTypedSelection must have strong pointers...

> We also need to something about the current use of |mShell|.
> AFAICT, it's a non-addref'ed plain pointer:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsFrameSelection.h&rev=1.19&root=/cvsroot&mark=639#639
> We need to handle the case were the nsFrameSelection out-lives the shell -
> which will be more likely now that we extend the life of the
> nsFrameSelection with strong refs.  Making |mShell| a nsWeakPtr is
> probably the way to go.
Right.

> Also, when dealing with frames, null-checking |mShell| isn't enough.
> For example:
> http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.296&root=/cvsroot&mark=3314,3324#3302
> the |tableLayoutObject| frame can be destroyed by line 3324.
> Holding a strong ref on |this| or even |mShell| doesn't help, we must
> use nsWeakFrame or lookup the frame again.
Right, will fix. Probably lookup the frame again.

> Maybe I'm missing something but I didn't quite get the point of moving
> nsTextInputSelectionImpl::mFrameSelection to a super-class and then
> using stack based strong refs... consumers of nsTextInputSelectionImpl
> (read nsISelectionController) are already required to have a strong ref,
> so mFrameSelection can't be destroyed...
nsTextInputSelectionImpl is owned by a nsIFrame object which may get deleted...

Comment 18

[Olli Pettay [:smaug]]

(In reply to comment #17)
> nsTextInputSelectionImpl is owned by a nsIFrame object which may get deleted...
Though, this shouldn't be a problem  here, except in ::SetValue.

Comment 19

[Olli Pettay [:smaug]]

Attachment: alternative approach, v2

Added DisconnectFromPresShell, null checks for mShell,
AddRef/Release implementation (based on the macro implementation) etc.

Comment 20

[Olli Pettay [:smaug]]

Attachment: v2

Comment 21

[Mats Palmgren (:mats)]

Comment on attachment 289540 [details] [diff] [review]
v2

Looks good. r=mats

A couple of nits:

>Index: layout/generic/nsSelection.cpp

While we're here, please remove the STATUS_CHECK_RETURN_MACRO macro and
replace it with NS_ENSURE_STATE(mShell) or add '!mShell' to the 'if':
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.296&root=/cvsroot&mark=2379#2369

>Index: layout/forms/nsTextControlFrame.cpp:
>+      nsCOMPtr<nsISelectionController> kungfuDeatGrip = mSelCon;

kungFuDeathGrip is the spelling used in other files...

Comment 22

[Mats Palmgren (:mats)]

Regarding the nsTypedSelection::AddRef()/Release() implementations:
It would be nice if had macros for the default bodies in nsISupportsImpl.h
so we could write something like:
nsTypedSelection::Release()
{
  if (mFrameSelection) {
    return mFrameSelection->Release();
  }
  NS_IMPL_RELEASE_WITH_DESTROY_BODY(nsTypedSelection, NS_DELETEXPCOM(this));
}

There are a few more places we could reuse that:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpccallcontext.cpp&rev=1.20&root=/cvsroot&mark=364-369#361
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/XPCDispPrivate.h&rev=1.24&root=/cvsroot&mark=128-137#125
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/ipc/ipcd/extensions/dconnect/src/ipcDConnectService.cpp&rev=1.14&root=/cvsroot&mark=983-992#977

Comment 23

[Olli Pettay [:smaug]]

(In reply to comment #22)
> Regarding the nsTypedSelection::AddRef()/Release() implementations:
> It would be nice if had macros for the default bodies in nsISupportsImpl.h

Could file a new bug for that.

Comment 24

[Olli Pettay [:smaug]]

Attachment: v2 + nits

Comment 25

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

The patch looks fine, but can we document which methods of nsFrameSelection can blow up the world? Because consumers like frames had better not call those methods. In fact it would be nice if there was a way to get a non-addrefed pointer to an object that implemented just the safe methods. Maybe we could make some of the safe methods in nsFrameSelection const and have methods that return non-addrefed but const nsFrameSelection pointers?

Comment 26

[Olli Pettay [:smaug]]

Attachment: const methods

Const methods help identifying problematic cases.
Not all non-const methods are unsafe, but when starting from
const nsFrameSelection*, one needs to think why to use non-const object.

Comment 27

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

Can you document in nsFrameSelection.h which methods are unsafe?

Comment 28

[Olli Pettay [:smaug]]

Attachment: added *unsafe*