488417 - Crash on table cell select (Ctrl+click) unselect [@ nsTypedSelection::RemoveRange(nsIRange*) ]

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Biju]

Attachment: table_select_crash.html

Steps:-
1. Open attached "table_select_crash.html"
2. ctrl + click on "A1" to select cell
3. ctrl + click on "B1" to select cell
4. Again ctrl + click on "A1" to unselect cell

Result
firefox crashes

Expected
"A1" unselected


PASS
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre)
Gecko/20090414 Shiretoko/3.5b4pre

CRASHES
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre)
Gecko/20090413 Minefield/3.6a1pre

Comment 1

[Matthias Versen [:Matti]]

Do you have a crash ID ?

Comment 2

[Biju]

ae92b3a9-2f61-467c-bdf1-e80762090415

Comment 3

[Matthias Versen [:Matti]]

0  	xul.dll  	nsTypedSelection::RemoveRange  	 layout/generic/nsSelection.cpp:5126
1 	xul.dll 	nsFrameSelection::HandleTableSelection 	layout/generic/nsSelection.cpp:2756
2 	xul.dll 	HandleFrameSelection 	layout/generic/nsFrame.cpp:2384
3 	xul.dll 	nsFrame::HandleRelease 	layout/generic/nsFrame.cpp:2461
4 	xul.dll 	nsFrame::HandleEvent 	layout/generic/nsFrame.cpp:1707
5 	xul.dll 	nsPresShellEventCB::HandleEvent 	layout/base/nsPresShell.cpp:1384
6 	xul.dll 	xul.dll@0x3c2a19

Comment 5

[Martijn Wargers (dead)]

This seems to have regressed somewhere between 2009-04-01 and 2009-04-12.
Also, see https://bugzilla.mozilla.org/attachment.cgi?id=372982 which is a test with enhanced privileges, which might be useful for an automated test.

Comment 6

[Graeme McCutcheon [:graememcc]]

Using Martijn's testcase from bug 488579.

Works: http://hg.mozilla.org/mozilla-central/rev/5506e9239176
Crashes: http://hg.mozilla.org/mozilla-central/rev/a10ff1269f64

Comment 7

[Graeme McCutcheon [:graememcc]]

So it looks like an AddRef has got lost somewhere.

nsTypedSelection::RemoveRange calls RemoveItem(aRange). When RemoveItem removes the range from mRanges, the refcount drops to 0, and gets destroyed. After that call, when aRange is dereferenced it is now a dangling pointer...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Yeah, indeed. This should do the trick

Comment 9

[Boris Zbarsky [:bzbarsky]]

Graeme, thanks for doing all the hard work here!

If someone can think of a way of automating that test, please let me know?

Comment 10

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

Comment on attachment 373432 [details] [diff] [review]
Yeah, indeed.  This should do the trick

A mochitest could just synthesize mouse events, right?

Comment 11

[Graeme McCutcheon [:graememcc]]

Why does this block 1.9.1?

It's a regression from 486547, which is trunk only AFAICT

Comment 12

[Boris Zbarsky [:bzbarsky]]

Yeah.  This has nothing to do with 1.9.1.  It's a regression from a trunk-only cleanup that's staying trunk-only.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/da575593579c with a mochitest.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I had to disable the mochitest, because it made the test _after_ it fail (more or less reliably on mac, intermittently on Windows, never on Linux).

roc, it looks like you wrote that test (test_movement_by_characters.html). Any idea why it's failing?

Comment 15

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

Sorry, no...

Comment 16

[Boris Zbarsky [:bzbarsky]]

And try server doesn't get the orange.  I have no idea what's going on with that, and I've filed bug 489560 on reenabling it.