336592 - Crash when window gets destroyed during selection event

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
When selecting some text in the textarea in the iframe, Mozilla crashes.

This is a regression, doesn't happen in 2005-03-06 build, happens in 2005-03-07 build.
This seems to me like a regression from bug 276892.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Talkback ID: TB18286783Y
nsTypedSelection::StopAutoScrollTimer   nsFrame::HandleEvent   nsPresShellEventCB::HandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventDispatcher::Dispatch   PresShell::HandleEventInternal   PresShell::HandlePositionedEvent   PresShell::HandleEvent   nsViewManager::HandleEvent   nsViewManager::DispatchEvent   HandleEvent   nsWindow::DispatchEvent   nsWindow::DispatchMouseEvent  

Comment 3

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

note, you get different kind of stack if you select only some part of the text.

Comment 4

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

Attachment: a patch

There are (at least) two cases here, I think. One is fixed by using strong ref to frameselections, for the other one it must be either ensured that frame may 
not get deleted while handling the event or checking whether the frame is still
alive after (activeFrame != this && ...) handling.
(The patch adds also some null checks to nsTextInputListener)

The patch just shows the idea. I'm really not familiar with this code, so 
someone might be able to fix the crash in a much simpler and not as hackish 
way.

Comment 5

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

Attachment: proposed patch

I think this should make sense :)
...at least for this crash.
Eventually we should create selection event in a bit different way.
Not creating it while handling an event in nsFrame.

Comment 6

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

This won't work in general because there could be a reframe that destroys the frame but doesn't destroy the presshell.

Comment 7

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

Comment on attachment 221637 [details] [diff] [review]
proposed patch

ah, true. :(
select event just fires in a wrong place and time.

Comment 8

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

I think you can make this function robust against frame destruction. How about moving

        nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, this);
        ContentOffsets offsets = GetContentOffsetsFromPoint(pt);

up to the top, and also moving

        result = GetDataForTableSelection(frameselection, shell, me,
                                          getter_AddRefs(parentContent),
                                          &contentOffset, &target);

and its dependencies up to the top? Then you won't use 'this' again. To make that clear you could do those things and then move the rest of the function to a static helper function that doesn't have access to the nsIFrame.

Comment 9

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

Attachment: like this?

So I moved all nsFrame dependent things to happen before (activeFrame != this)
and other stuff are now in (static) HandleFrameSelection.
Looks a bit ugly though :(

Comment 10

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

Comment on attachment 221688 [details] [diff] [review]
like this?

+                     nsIFrame::ContentOffsets* aOffsets,

Make this a const &.

Yes, it's a bit ugly, but it's localized ugliness so I'm not worried about it.

Comment 11

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

(In reply to comment #10)
> (From update of attachment 221688 [details] [diff] [review] [edit])
> +                     nsIFrame::ContentOffsets* aOffsets,
> 
> Make this a const &.
> 


That doesn't compile.

Comment 12

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

I'll do it without const.

Comment 13

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

OK

Comment 14

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

Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.648; previous revision: 3.647
done

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-05-12-07 of SeaMonkey trunk on Windows XP with testcase: https://bugzilla.mozilla.org/attachment.cgi?id=220772&action=view.

No crash.

Comment 16

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

Attachment: for 1.8

This fixes also bug 346431.
The changes to nsSelection.cpp are just backing out part of the fix for bug 
345600. Those aren't needed anymore and this way bug 346964 gets fixed.

Comment 17

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

Attachment: PRUint32 -> PRInt32 (for trunk)

This was wrong in trunk patch.

Comment 18

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

Not a blocker, but we'll consider the patch.

Comment 19

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

Comment on attachment 231936 [details] [diff] [review]
for 1.8

This patch fixes also the blocking1.8.1+ bug 346431.

Comment 20

[Mike Schroepfer]

Comment on attachment 231936 [details] [diff] [review]
for 1.8

a=schrep for drivers.

Comment 21

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

Because this fixes Bug 346431, we should probably take this to 1.8.0.7.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 231936 [details] [diff] [review]
for 1.8

approved for 1.8.0 branch, a=dveditz for drivers

Comment 23

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

Attachment: for 1.8.0

This is almost the same as for 1.8, but since 1.8 had 
if (NS_FAILED(rv)) { return rv; }, and 1.8 patch is also backing out part of 
Bug 345600, 1.8 doesn't apply cleanly.

Comment 24

[alice nodelman [:alice] [:anode]]

using testcase in comment #1

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060825 Firefox/1.5.0.7pre

verified 1.5.0.7

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b2) Gecko/20060825 BonEcho/2.0b2

verified 1.8.1b2