308086 - sometimes there is a crash when *:focus{display:none;} is set and I start tabbing around [@ nsPlaceholderFrame::GetRealFrameFor]

Status: VERIFIED FIXED

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

Description

[Grant Wohl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050911 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050911 Firefox/1.4

Sometimes, when *:focus{display:none;} is set, when I press the tab key, there
is a crash.

Reproducible: Sometimes

Steps to Reproduce:
1.Load attached testcase.
2.Follow the instructions.

Actual Results:  
Crash.

Expected Results:  
It should have focused the link, realized it was focused and apply display:none,
and not crash.

Comment 1

[Grant Wohl]

Attachment: testcase

I can reproduce it every time with this testcase.

Comment 2

[Grant Wohl]

Here are some talkback IDs: TB9258376M, TB9258376M, TB9258376M, and TB9258122H.

Comment 3

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

nsPlaceholderFrame::GetRealFrameFor  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/../generic\nsPlaceholderFrame.h, line 109]
nsFrameTraversal::NewFrameTraversal  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameTraversal.cpp, line 266]
nsEventStateManager::ShiftFocusInternal  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventStateManager.cpp, line 3234]
nsEventStateManager::ShiftFocus  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventStateManager.cpp, line 3117]
nsEventStateManager::PostHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventStateManager.cpp, line 2099]
PresShell::HandleEventInternal  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 6081]
PresShell::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 5863]
nsViewManager::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp, line 2504]
nsViewManager::DispatchEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp, line 2237]
HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp, line 176]
nsWindow::DispatchEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 1162]
nsWindow::DispatchKeyEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 3372]
nsWindow::OnChar  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 3618]
nsWindow::OnKeyDown  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 3464]
nsWindow::ProcessMessage  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 4453]
nsWindow::WindowProc  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp, line 1351]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0x89cd (0x77d489cd)
USER32.dll + 0x8a10 (0x77d48a10)
nsAppShell::Run  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp, line 159]
nsAppStartup::Run  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/components/startup/src/nsAppStartup.cpp, line 208]
main  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1737]
WinMain  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1761]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the issue here is that in nsEventStateManager::ShiftFocusInternal we find that we have a selection, call MoveFocusToCaret() (which destroys the selectionFrame in this case), then start doing things with selectionFrame.  That's not good.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Aaron, roc, thoughts?  Would just regetting the selectionFrame from the selectionContent after the MoveFocusToCaret() call work?  Or should we call  GetDocSelectionLocation again (since it might give us a different answer this time)?  I'd hope that MoveFocusToCaret() doesn't fire any DOM events, but I'm not sure.... :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and if there is a safe-ish way to fix this on 1.8, we should -- this is probably exploitable, since GetType() is a virtual method.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Testcase #2

The first testcase makes the new GetDocSelectionLocation() frame become null.
This testcase makes it non-null and different from the originalGetDocSelectionLocation() frame.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Stack

FWIW, this is the stack when the original GetDocSelectionLocation() frame
is destroyed.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Call GetDocSelectionLocation() again after MoveFocusToCaret() to get
an up-to-date frame. I guess we could assign it to a member variable
and then null it in ClearFrameRefs() to avoid the second call in most
cases, but I don't think this path is performance sensitive.

Comment 10

[Mats Palmgren (inactive)]

(In reply to comment #5)
> I'd hope that MoveFocusToCaret() doesn't fire any DOM events, but I'm
> not sure.... :(
> 

No, it doesn't as far as I can tell.
It's nsHTMLAnchorElement::SetFocus() that flushes a pending style change
that recreates the frames. See attachment 206219 [details]. 

Comment 11

[Mats Palmgren (inactive)]

Potentially security-sensitive according to comment 6.

Comment 12

[Daniel Veditz [:dveditz]]

bryner: please review, we're considering this for ff1.5.0.1

Comment 13

[Mats Palmgren (inactive)]

Checked in on trunk at 2006-01-06 20:08 PDT

-> FIXED

Comment 14

[Mike Schroepfer]

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

a=schrep for drivers - need more time on the trunk to double-check for regressions.  Re-nominatingfor 1.8.0.2 as it should go in for that release.

Comment 15

[Mats Palmgren (inactive)]

Commited to MOZILLA_1_8_BRANCH at 2006-01-11 19:50 PDT:

Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.595.2.12; previous revision: 1.595.2.11
done

Comment 16

[Bob Clary [:bc] (inactive)]

v 20060112 linux 1.8.1, 20060113 windows 1.8.1

Comment 17

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 18

[Mats Palmgren (inactive)]

Checked in on MOZILLA_1_8_0_BRANCH at 2006-02-16 21:52 PDT

/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.595.2.10.2.2; previous revision: 1.595.2.10.2.1

Comment 19

[Daniel Veditz [:dveditz]]

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

Do we want this on the aviary101/moz17 branches? The crash with the first testcase looks relatively benign in 1.0.x and I couldn't get the second testcase to crash, but this patch definitely solves the problem.

Comment 20

[Daniel Veditz [:dveditz]]

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

Do we want this on the aviary101/moz17 branches? The crash with the first testcase looks relatively benign in 1.0.x and I couldn't get the second testcase to crash, but this patch definitely solves the problem.

Comment 21

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, no crash with either testcase.

Comment 22

[Daniel Veditz [:dveditz]]

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

Too late for 1.0.8 w/out verification that it's a problem there. Try for 1.0.9