402198 - Crash [@ nsHTMLSelectElement::PreHandleEvent] with shift-tabbing bunch of selects and stuff and setting display: none

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P4)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes after 300ms after load. Because of the use of enhanced privileges, you need to download the testcase to your computer.

This seems to have regressed between 2007-08-21 and 2007-08-22:
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=2007-08-21+09&maxdate=2007-08-22+07&cvsroot=%2Fcvsroot

http://crash-stats.mozilla.com/report/index/b26f0b4c-8942-11dc-a5f7-001a4bd43ef6
0  	@0x0  	
1 	nsHTMLSelectElement::PreHandleEvent(nsEventChainPreVisitor&) 	mozilla/content/html/content/src/nsHTMLSelectElement.cpp:1480
2 	nsEventTargetChainItem::PreHandleEvent(nsEventChainPreVisitor&) 	mozilla/content/events/src/nsEventDispatcher.cpp:186
3 	nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*) 	mozilla/content/events/src/nsEventDispatcher.cpp:436
4 	nsEventStateManager::SendFocusBlur(nsPresContext*, nsIContent*, int) 	mozilla/content/events/src/nsEventStateManager.cpp:4485
5 	nsEventStateManager::SetContentState(nsIContent*, int) 	mozilla/content/events/src/nsEventStateManager.cpp:4194
6 	nsGenericHTMLFormElement::SetFocusAndScrollIntoView(nsPresContext*) 	mozilla/content/html/content/src/nsGenericHTMLElement.cpp:2891
7 	nsHTMLButtonElement::SetFocus(nsPresContext*) 	mozilla/content/html/content/src/nsHTMLButtonElement.cpp:269
8 	nsEventStateManager::ChangeFocusWith(nsIContent*, nsIEventStateManager::EFocusedWithType) 	mozilla/content/events/src/nsEventStateManager.cpp:3342
9 	nsEventStateManager::ShiftFocusInternal(int, nsIContent*) 	mozilla/content/events/src/nsEventStateManager.cpp:3627
10 	nsEventStateManager::ShiftFocus(int, nsIContent*) 	mozilla/content/events/src/nsEventStateManager.cpp:3423
11 	nsEventStateManager::PostHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*, nsIView*) 	mozilla/content/events/src/nsEventStateManager.cpp:2513
12 	PresShell::HandleEventInternal(nsEvent*, nsIView*, nsEventStatus*) 	mozilla/layout/base/nsPresShell.cpp:5797
13 	PresShell::HandleEvent(nsIView*, nsGUIEvent*, nsEventStatus*) 	mozilla/layout/base/nsPresShell.cpp:5577
14 	nsViewManager::HandleEvent(nsView*, nsPoint, nsGUIEvent*, int) 	mozilla/view/src/nsViewManager.cpp:1296
15 	nsViewManager::DispatchEvent(nsGUIEvent*, nsEventStatus*) 	mozilla/view/src/nsViewManager.cpp:1252
16 	HandleEvent 	mozilla/view/src/nsView.cpp:168
17 	nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) 	mozilla/widget/src/windows/nsWindow.cpp:1051
18 	nsDOMWindowUtils::SendKeyEvent(nsAString_internal const&, int, int, int) 	mozilla/dom/src/base/nsDOMWindowUtils.cpp:276
19 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
20 	AutoJSSuspendRequest::SuspendRequest() 	mozilla/js/src/xpconnect/src/xpcprivate.h:3328
21 	CalculateBitmapSize 	mozilla/js/src/jsregexp.c:954

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase2

This is a testcase that can crash online, when you press shift-tab.
(note that I desperately tried to minimize the testcases further, to no avail)

Comment 2

[Mats Palmgren (inactive)]

Attachment: Stack trace for the bad frame destruction

It's this Invalidate() call that destroys |this|:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.419&root=/cvsroot&mark=362#336

Comment 3

[Mats Palmgren (inactive)]

Attachment: wip

This fixes the crash.  nsComboboxControlFrame::SetFocus() is already known
to potentially destroy the frame, so its callers should already be ok,
but I think there is some list frame code that also invalidates that
needs to be investigated...  more in a bit...

Comment 4

[Mats Palmgren (inactive)]

Attachment: Mochitest based on first attachment

Comment 5

[Mats Palmgren (inactive)]

This was the call I worried about:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsListControlFrame.cpp&rev=1.433&root=/cvsroot&mark=392#378
but I think it should be ok since it uses the PR_FALSE default value
for |aImmediate| so we can assume no flush will occur (I think).

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

nsComboboxControlFrame::SetFocus() is already known to potentially destroy
the frame, so its callers should already be prepared for that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Looks ok, but roc should look too.

Can we just eliminate the sync invalidates?  They seem to be a bad idea.

Comment 8

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

+  // This call might destroy |this|.
   Invalidate(nsRect(0,0,mRect.width,mRect.height), PR_TRUE);
 
+  if (!weakFrame.IsAlive()) {
+    return;
+  }

What bz said ... just make this Invalidate async (PR_FALSE) instead.

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Comment 10

[Jesse Ruderman]

This is one of the two bugs (along with bug 402853) still suspected of causing the prolonged orange on the Windows unit test boxes over the weekend (bug 403271 comment 7).  That orange could only be fixed by doing a srcdir clobber, so please reland this at a time when robcee is around (coordinate using #developers).

Comment 11

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

I would very very surprised if this patch caused orange. I believe it's super-safe.

Comment 12

[Reed Loden [:reed]]

Relanded the backed out parts of this patch.

Checking in layout/forms/nsComboboxControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsComboboxControlFrame.cpp,v  <--  nsComboboxControlFrame.cpp
new revision: 1.422; previous revision: 1.421
done
Checking in layout/forms/test/Makefile.in;
/cvsroot/mozilla/layout/forms/test/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done

Comment 13

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111404 Minefield/3.0b2pre

Comment 14

[Mats Palmgren (inactive)]

(the testcase landed in comment 12)