Closed Bug 368760 Opened 14 years ago Closed 13 years ago

NotifySelectionListeners might lead to self destruction

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: mats, Assigned: smaug)

References

Details

(Whiteboard: [sg:critical?][dbaron-1.9:RsCe])

Attachments

(1 file, 6 obsolete files)

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...
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
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.
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
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Do NotifySelectionListeners() off an event.
Attachment #254524 - Flags: superreview?(bzbarsky)
Attachment #254524 - Flags: review?(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?
Attachment #254524 - Flags: review?(bzbarsky) → review?(sfraser_bugs)
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.
This is old code so potentially affects the 1.8 branch as well, right?
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Flags: blocking1.9?
Whiteboard: [sg:critical?]
Flags: blocking1.9? → blocking1.9+
Blocks: 372676
Comment on attachment 254524 [details] [diff] [review]
Patch rev. 1

Please rerequest sr once this has review...
Attachment #254524 - Flags: superreview?(bzbarsky)
Mats, can you please address Comment #6 ?
Seconding Roc's request for followup.  :)
Whiteboard: [sg:critical?] → [sg:critical?][dbaron-1.9:RsCe]
This is a scary area with possible regressions and stuff, so we really need it addressed for beta2.
Priority: -- → P1
Smaug, any chance you'd be able to pick this one up and figure out what to do here?
Attached patch alternative approach (obsolete) — Splinter Review
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?
Attachment #289462 - Flags: review?(mats.palmgren)
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.
Attachment #289462 - Flags: review?(mats.palmgren) → review-
Assignee: mats.palmgren → Olli.Pettay
Attachment #254524 - Attachment is obsolete: true
Attachment #254524 - Flags: review?(sfraser_bugs)
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.
(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...
(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.

Attached patch alternative approach, v2 (obsolete) — Splinter Review
Added DisconnectFromPresShell, null checks for mShell,
AddRef/Release implementation (based on the macro implementation) etc.
Attachment #289462 - Attachment is obsolete: true
Attachment #289537 - Flags: review?(mats.palmgren)
Attached patch v2 (obsolete) — Splinter Review
Attachment #289537 - Attachment is obsolete: true
Attachment #289540 - Flags: review?(mats.palmgren)
Attachment #289537 - Flags: review?(mats.palmgren)
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...
Attachment #289540 - Flags: review?(mats.palmgren) → review+
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
(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.
Attached patch v2 + nits (obsolete) — Splinter Review
Attachment #289540 - Attachment is obsolete: true
Attachment #289649 - Flags: superreview?(roc)
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?
Attached patch const methods (obsolete) — Splinter Review
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.
Attachment #289649 - Attachment is obsolete: true
Attachment #289835 - Flags: superreview?(roc)
Attachment #289649 - Flags: superreview?(roc)
Can you document in nsFrameSelection.h which methods are unsafe?
Attached patch added *unsafe*Splinter Review
Attachment #289835 - Attachment is obsolete: true
Attachment #289839 - Flags: superreview?(roc)
Attachment #289835 - Flags: superreview?(roc)
Attachment #289839 - Flags: superreview?(roc) → superreview+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 427647
Group: core-security
You need to log in before you can comment on or make changes to this bug.