Crash when window gets destroyed during selection event

VERIFIED FIXED

Status

()

--
critical
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: martijn.martijn, Assigned: smaug)

Tracking

(5 keywords)

Trunk
x86
Windows XP
crash, regression, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -
blocking1.8.0.7 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 220772 [details]
testcase
(Reporter)

Comment 2

13 years ago
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  
(Assignee)

Comment 3

13 years ago
note, you get different kind of stack if you select only some part of the text.
(Assignee)

Comment 4

13 years ago
Created attachment 221589 [details] [diff] [review]
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.
(Assignee)

Comment 5

13 years ago
Created attachment 221637 [details] [diff] [review]
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.
Assignee: events → Olli.Pettay
Attachment #221589 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #221637 - Flags: superreview?(roc)
Attachment #221637 - Flags: review?(roc)
This won't work in general because there could be a reframe that destroys the frame but doesn't destroy the presshell.
(Assignee)

Comment 7

13 years ago
Comment on attachment 221637 [details] [diff] [review]
proposed patch

ah, true. :(
select event just fires in a wrong place and time.
Attachment #221637 - Flags: superreview?(roc)
Attachment #221637 - Flags: review?(roc)
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.
(Assignee)

Comment 9

13 years ago
Created attachment 221688 [details] [diff] [review]
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 :(
Attachment #221688 - Flags: superreview?(roc)
Attachment #221688 - Flags: review?(roc)
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.
Attachment #221688 - Flags: superreview?(roc)
Attachment #221688 - Flags: superreview+
Attachment #221688 - Flags: review?(roc)
Attachment #221688 - Flags: review+
(Assignee)

Comment 11

13 years ago
(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.
(Assignee)

Comment 12

13 years ago
I'll do it without const.
(Assignee)

Comment 14

13 years ago
Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.648; previous revision: 3.647
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

12 years ago
Blocks: 346431
(Assignee)

Updated

12 years ago
Blocks: 346964
(Assignee)

Comment 16

12 years ago
Created attachment 231936 [details] [diff] [review]
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.
Attachment #231936 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Flags: blocking1.8.1?
(Assignee)

Comment 17

12 years ago
Created attachment 231993 [details] [diff] [review]
PRUint32 -> PRInt32 (for trunk)

This was wrong in trunk patch.
Attachment #231993 - Flags: superreview?(roc)
Attachment #231993 - Flags: review?(roc)
Not a blocker, but we'll consider the patch.
Flags: blocking1.8.1? → blocking1.8.1-
Attachment #231993 - Flags: superreview?(roc)
Attachment #231993 - Flags: superreview+
Attachment #231993 - Flags: review?(roc)
Attachment #231993 - Flags: review+
Attachment #231936 - Flags: superreview+
Attachment #231936 - Flags: review?(roc)
Attachment #231936 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #231936 - Flags: approval1.8.1?
(Assignee)

Comment 19

12 years ago
Comment on attachment 231936 [details] [diff] [review]
for 1.8

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

Comment 20

12 years ago
Comment on attachment 231936 [details] [diff] [review]
for 1.8

a=schrep for drivers.
Attachment #231936 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.1
(Assignee)

Comment 21

12 years ago
Because this fixes Bug 346431, we should probably take this to 1.8.0.7.
Flags: blocking1.8.0.7?
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Comment on attachment 231936 [details] [diff] [review]
for 1.8

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #231936 - Flags: approval1.8.0.7+
(Assignee)

Comment 23

12 years ago
Created attachment 233553 [details] [diff] [review]
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.
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.0.7
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
Keywords: fixed1.8.0.7, fixed1.8.1 → verified1.8.0.7, verified1.8.1
You need to log in before you can comment on or make changes to this bug.