Closed Bug 404192 Opened 17 years ago Closed 16 years ago

Crash [@ nsCaret::UpdateCaretRects] with caret browsing, xul titlebar focusing and blurring

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: roc)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(2 files, 1 obsolete file)

Attached file testcase
See testcase, which crashes after 200ms if you have caret browsing turned on.

This regressed between 2007-08-12 and 2007-08-13:
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-12+04&maxdate=2007-08-13+21&cvsroot=%2Fcvsroot
My guess is a regression from bug 335560.
http://crash-stats.mozilla.com/report/index/bd8b8c66-9546-11dc-b9c5-001a4bd43e5c
0  	nsCaret::UpdateCaretRects(nsIFrame*, int)  	 mozilla/layout/base/nsCaret.cpp:1113
1 	nsCaret::DrawAtPositionWithHint(nsIDOMNode*, int, nsFrameSelection::HINT, unsigned char, int) 	mozilla/layout/base/nsCaret.cpp:601
2 	nsCaret::DrawCaret(int) 	mozilla/layout/base/nsCaret.cpp:1036
3 	nsCaret::StartBlinking() 	mozilla/layout/base/nsCaret.cpp:537
4 	nsCaret::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) 	mozilla/layout/base/nsCaret.cpp:480
5 	nsTypedSelection::NotifySelectionListeners() 	mozilla/layout/generic/nsSelection.cpp:7412
6 	nsFrameSelection::NotifySelectionListeners(short) 	mozilla/layout/generic/nsSelection.cpp:2859
7 	nsTypedSelection::AddRange(nsIDOMRange*) 	mozilla/layout/generic/nsSelection.cpp:5777
8 	nsEventStateManager::MoveCaretToFocus() 	mozilla/content/events/src/nsEventStateManager.cpp:5313
9 	nsGenericHTMLElement::SetElementFocus(int) 	mozilla/content/html/content/src/nsGenericHTMLElement.cpp:3086
10 	nsGenericHTMLElement::Focus() 	mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp:300
11 	nsGenericHTMLElementTearoff::Focus() 	mozilla/content/html/content/src/nsGenericHTMLElement.cpp:196
12 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
13 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2339
14 	GetPropertyTreeChild 	mozilla/js/src/jsscope.c:912
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
I can't get this to crash for me at all.
Still crashes for me in current trunk build (don't forget the caret browsing part).
Attached patch Null defense (obsolete) — Splinter Review
I must admit that I don't really understand the underlying cause here, and the result on this page is not really ideal (the caret ends up sticking out of the upper left corner of the page) but at least with this patch we don't crash.

We end up with an nsScrollPortView with no first child, which means that GetScrolledView returns null. roc, is this good enough, or do I need to dig deeper to figure out why there's no scrolled view?
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #294303 - Flags: superreview?(roc)
Attachment #294303 - Flags: review?(roc)
Oh, and I was totally forgetting the caret browsing part. Thanks for the heads up!
Whiteboard: [patch]
I would really like to know why there's no scrolled view. That probably means a corrupt frame hierarchy leading to other problems.
Giving this to roc to understand how we end up without a scrolled view.

I see us create a nsXULScrollFrame as a result of dealing with the xul:titlebar but we never call SetScrolledView on the resulting view. Then (and I haven't confirmed this) we try to focus the HTML element, which is impossible, so we select its first child, which is the nsXULScrollFrame, leading to the crash (since there's no scrolled view). FWIW, this all seems reasonable to me, as I understand things, there's no reason to create a view for the titlebar until we actually need to scroll it.
Assignee: mrbkap → roc
Status: ASSIGNED → NEW
The titlebar frame creates its own view which becomes the first child of the scrollframe's view. The actual scrollable view becomes the second child of the scrollframe's view. Whee!
Attached patch fixSplinter Review
Simple fix. This got out of sync with nsHTMLContainerFrame::CreateViewForFrame.

The code duplication is unfortunate but will go away when we get rid of views post-1.9.
Attachment #294303 - Attachment is obsolete: true
Attachment #295714 - Flags: superreview?(mats.palmgren)
Attachment #295714 - Flags: review?(mats.palmgren)
Attachment #294303 - Flags: superreview?(roc)
Attachment #294303 - Flags: review?(roc)
Whiteboard: [patch] → [needs review]
Comment on attachment 295714 [details] [diff] [review]
fix

Looks good, r+sr=mats

Maybe add a comment to both nsHTMLContainerFrame::CreateViewForFrame
and nsBoxFrame::CreateViewForFrame saying that "please keep this in
sync with ..." ?
Attachment #295714 - Flags: superreview?(mats.palmgren)
Attachment #295714 - Flags: superreview+
Attachment #295714 - Flags: review?(mats.palmgren)
Attachment #295714 - Flags: review+
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [needs review]
sure
Whiteboard: [needs landing]
checked in with that comment.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Whiteboard: [needs landing]
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012204 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsCaret::UpdateCaretRects]
Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30ea82feef41
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: