345155 - crash [@ nsEventStateManager::GetDocSelectionLocation]

Status: VERIFIED FIXED

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

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. load the attached testcase
2. click on the button
3. TAB twice  -> crash

PLATFORMS AND BUILDS TESTED
bug occurs with SeaMonkey trunk debug build on Linux

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: stack

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes the crash but I'm not sure if it's actually the |domRange|
or root content, or something else that is incorrect... we have:

(gdb) fr 0
#0  0x4137d69e in nsEventStateManager::GetDocSelectionLocation(nsIContent**, nsIContent**, nsIFrame**, unsigned*) (this=0x88559c0,
    aStartContent=0xbfffd3e0, aEndContent=0xbfffd3d0, aStartFrame=0xbfffd2cc, aStartOffset=0xbfffd2d0) at nsEventStateManager.cpp:4783
4783          if (startContent->IsNodeOfType(nsINode::eELEMENT)) {
(gdb) p startNode
$6 = {mRawPtr = 0x83ce434}
(gdb) x/wa startNode.mRawPtr
0x83ce434:      0x4173c6c8 <_ZTV14nsHTMLDocument+1224>
(gdb) fr 1
#1  0x41379799 in nsEventStateManager::ShiftFocusInternal(int, nsIContent*) (this=0x88559c0, aForward=1, aStart=0x0)
    at nsEventStateManager.cpp:3406
3406          GetDocSelectionLocation(getter_AddRefs(selectionContent), getter_AddRefs(endSelectionContent), &selectionFrame, &selectionOffset);
(gdb) list
3401      if (!aStart && itemType != nsIDocShellTreeItem::typeChrome) {
3402        // We're going to tab from the selection position
3403        if (!mCurrentFocus || (mLastFocusedWith != eEventFocusedByMouse && mCurrentFocus->Tag() != nsHTMLAtoms::area)) {
3404          nsCOMPtr<nsIContent> selectionContent, endSelectionContent;  // We won't be using this, need arg for method call
3405          PRUint32 selectionOffset; // We won't be using this either, need arg for method call
3406          GetDocSelectionLocation(getter_AddRefs(selectionContent), getter_AddRefs(endSelectionContent), &selectionFrame, &selectionOffset);
3407          if (selectionContent == rootContent)  // If selection on rootContent, same as null -- we have no selection yet
3408            selectionFrame = nsnull;
3409          // Only use tabindex if selection is synchronized with focus
3410          // That way, if the user clicks in content, or does a find text that lands between focusable elements,
(gdb) p rootContent
$7 = {mRawPtr = 0x884d590}
(gdb) x/wa rootContent.mRawPtr
0x884d590:      0x41731a28 <_ZTV18nsHTMLInputElement+8>

Comment 4

[Ria Klaassen (not reading all bugmail)]

Happens also on windows.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 229785 [details] [diff] [review]
Patch rev. 1

This is the right fix, esp. for branch.  The range looks correct to me -- removing the root node will collapse all ranges to the Document node, which doesn't QI to nsIContent, of course.  The root content also looks correct.

That said, this code should probably be rewritten to use nsINode instead of nsIContent on trunk (at which point we could remove these null-checks).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and we probably need this on branches too...

Comment 7

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-07-19 22:25 PDT.
Filed bug 345278 on using nsINode instead of nsIContent.

-> FIXED

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 229785 [details] [diff] [review]
Patch rev. 1

(In reply to comment #6)
> Oh, and we probably need this on branches too...

It doesn't crash in Fx2.0b1 or a local SeaMonkey branch debug build on Linux.
The reason being that |domRange| is null:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/events/src/nsEventStateManager.cpp&rev=MOZILLA_1_8_BRANCH&cvsroot=/cvsroot&mark=4819#4782

Maybe we should take it just in case that changes...
It's a simple null-check fix, zero risk.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229785 [details] [diff] [review]
Patch rev. 1

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.

Comment 10

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH at 2006-07-28 11:45 PDT.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 229785 [details] [diff] [review]
Patch rev. 1

approved for 1.8.0 branch, a=dveditz for drivers

Comment 12

[Adam Guthrie]

I guess this wasn't happening before the patch went in, but nevertheless, it still isn't happening. Verified using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060727 BonEcho/2.0b1 and the testcase from comment 1.

Comment 13

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_0_BRANCH at 2006-08-21 10:49 PDT.

Comment 14

[Martijn Wargers (dead)]

Same story for the 1.5.0.7 branch.
I don't get the crash with Firefox1.5.0.6, and I still don't get the crash, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060829 Firefox/1.5.0.7