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

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[Martijn Wargers (dead)]

Attachment: 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.

Comment 1

[Martijn Wargers (dead)]

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

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I can't get this to crash for me at all.

Comment 3

[Martijn Wargers (dead)]

Still crashes for me in current trunk build (don't forget the caret browsing part).

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Null defense

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?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Oh, and I was totally forgetting the caret browsing part. Thanks for the heads up!

Comment 6

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

I would really like to know why there's no scrolled view. That probably means a corrupt frame hierarchy leading to other problems.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 8

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

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!

Comment 9

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

Attachment: fix

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.

Comment 10

[Mats Palmgren (inactive)]

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 ..." ?

Comment 11

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

sure

Comment 12

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

checked in with that comment.

Comment 13

[Martijn Wargers (dead)]

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

Comment 14

[Mats Palmgren (inactive)]

Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30ea82feef41

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/30ea82feef41