Closed
Bug 402198
Opened 17 years ago
Closed 17 years ago
Crash [@ nsHTMLSelectElement::PreHandleEvent] with shift-tabbing bunch of selects and stuff and setting display: none
Categories
(Core :: Layout: Form Controls, defect, P4)
Tracking
()
VERIFIED
FIXED
mozilla1.9beta2
People
(Reporter: martijn.martijn, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: crash, regression, testcase)
Crash Data
Attachments
(5 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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
Flags: blocking1.9?
Reporter | ||
Comment 1•17 years ago
|
||
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)
Assignee | ||
Comment 2•17 years ago
|
||
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
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•17 years ago
|
||
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...
Assignee | ||
Comment 4•17 years ago
|
||
Assignee | ||
Comment 5•17 years ago
|
||
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).
Flags: in-testsuite?
OS: Windows XP → All
Target Milestone: --- → mozilla1.9 M10
Assignee | ||
Comment 6•17 years ago
|
||
nsComboboxControlFrame::SetFocus() is already known to potentially destroy
the frame, so its callers should already be prepared for that.
Attachment #287110 -
Attachment is obsolete: true
Attachment #287119 -
Flags: superreview?(bzbarsky)
Attachment #287119 -
Flags: review?(bzbarsky)
Comment 7•17 years ago
|
||
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.
Attachment #287119 -
Flags: superreview?(roc)
Attachment #287119 -
Flags: superreview?(bzbarsky)
Attachment #287119 -
Flags: review?(bzbarsky)
Attachment #287119 -
Flags: review+
+ // 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.
Assignee | ||
Comment 9•17 years ago
|
||
Attachment #287119 -
Attachment is obsolete: true
Attachment #287345 -
Flags: superreview?(roc)
Attachment #287345 -
Flags: review?(roc)
Attachment #287119 -
Flags: superreview?(roc)
Attachment #287345 -
Flags: superreview?(roc)
Attachment #287345 -
Flags: superreview+
Attachment #287345 -
Flags: review?(roc)
Attachment #287345 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Component: Event Handling → Layout: Form Controls
Assignee | ||
Updated•17 years ago
|
Attachment #287345 -
Flags: approval1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Updated•17 years ago
|
Attachment #287345 -
Flags: approval1.9? → approval1.9+
Updated•17 years ago
|
Keywords: checkin-needed
Comment 10•17 years ago
|
||
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).
I would very very surprised if this patch caused orange. I believe it's super-safe.
Comment 12•17 years ago
|
||
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
Depends on: 403724
Reporter | ||
Comment 13•17 years ago
|
||
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111404 Minefield/3.0b2pre
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Crash Signature: [@ nsHTMLSelectElement::PreHandleEvent]
Assignee | ||
Comment 14•10 years ago
|
||
(the testcase landed in comment 12)
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•