349931 - Crash [@ nsDocument::GetFirstChild] when following selection instructions in this particular testcase

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

See upcoming testcase.
To reproduce the crash:
- Mousedown and drag the mouse from the right of the iframe into the iframe, then release the mouse button
- Now mousedown and drag the mouse from in the iframe to outside of the iframe, then release the mouse button

Talkback ID: TB22425496G
nsDocument::GetFirstChild   nsDOMEvent::GetType   nsFrameSelection::NotifySelectionListeners   nsFrameSelection::HandleClick   nsFrameSelection::HandleDrag   nsFrame::HandleDrag   nsFrame::HandleEvent   nsPresShellEventCB::HandleEvent   nsEventDispatcher::Dispatch   PresShell::HandleEventInternal   PresShell::HandlePositionedEvent   PresShell::HandleEvent   nsViewManager::HandleEvent   nsViewManager::DispatchEvent   HandleEvent   nsWindow::DispatchEvent   nsWindow::DispatchMouseEvent  

This regressed between 2006-02-03 and 2006-02-04:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-02-03+06&maxdate=2006-02-04+07&cvsroot=%2Fcvsroot
I suspect this is a regression from bug 253076.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Alex Vincent [:WeirdAl]]

I have to admit, I don't yet see a connection between nsDocument::GetFirstChild and nsDOMEvent::GetType.  Despite the stack trace, nsDOMEvent::GetType doesn't seem to call on nsDocument::GetFirstChild.

I'm building a SeaMonkey trunk now to see if I can reproduce and debug this.

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: stack traces for testcase

Okay, I was right - the stack trace given by Talkback is no good.

I find this testcase works best when you aim for just inside the lower right corner of the iframe.

It's also worth noting we hit an assertion:
###!!! ASSERTION: index out of range: '0 <= aIndex && aIndex < Count()', file m:\xpathgen\mozilla\xpcom\glue\nsVoidArray.h, line 72

The assertion is hit right before we crash.

Comment 4

[Eli Friedman]

Attachment: Revised test

Try figuring out the regression range with this revised test (this version puts a <br> in the iframe).

I don't have many builds downloaded, but it regressed between 20050827 and 20051009.

Comment 5

[jpl24]

So the crash happens while iterating over the selection listeners in nsTypedSelection. One of the listeners is a ResizerSelectionListener which can call nsHTMLEditor::GetAbsolutelyPositionedSelectionContainer, which gets a computed CSS property. This causes a flush, and wipes out the selection listeners in nsTypedSelection. Then the crash occurs when nsTypedSelection tries to loop over listeners that no longer exists. I'll attach a stack showing this and the debugging patch I used to find it.

I think the real issue is getting a computed property from inside a frame method, right?

Comment 6

[jpl24]

Attachment: Stack trace showing where the listener was removed

Comment 7

[jpl24]

Attachment: Patch that throws an assertion where the removal occurs

Comment 8

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

This is Windows only?
Would adding
nsRefPtr<nsTypedSelection> selection = mDomSelections[index];
return selection->NotifySelectionListeners();
to http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#2841
help?

Comment 9

[Martijn Wargers (dead)]

(In reply to comment #4)
> Created an attachment (id=235187) [edit]
> Revised test
> 
> Try figuring out the regression range with this revised test (this version puts
> a <br> in the iframe).

I tried crashing with this testcase, but I don't succeed in crashing with this. (sorry for the delay)

Comment 10

[Martijn Wargers (dead)]

(In reply to comment #8)
> This is Windows only?
> Would adding
> nsRefPtr<nsTypedSelection> selection = mDomSelections[index];
> return selection->NotifySelectionListeners();
> to http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSelection.cpp#2841
> help?

I just tried it in my debug build, no, that doesn't seem to help. I still crash with the testcase, after I changed that code.

Comment 11

[Martijn Wargers (dead)]

Comment on attachment 235129 [details]
testcase

I'm unobsoluting this testcase, since I am not able to crash with the other testcase.

Comment 12

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

I managed to crash after all. OS->All

Comment 13

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

Attachment: something like this?

I'm not too happy to use weakframe here.
Roc, any other ideas?

Comment 14

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

Comment on attachment 238194 [details] [diff] [review]
something like this?

no, not really.

Comment 15

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

Checked in.
The first testcase shouldn't crash anymore.
If there are still problems with the second test, please reopen this bug
or file a new one. (I managed to crash only using the first testcase.)

Comment 16

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

Verified FIXED for me with build 2006-09-19-09 of SeaMonkey trunk under Windows XP with:
https://bugzilla.mozilla.org/attachment.cgi?id=235129&action=view and
https://bugzilla.mozilla.org/attachment.cgi?id=235187 as the testcases.

No crash.