770580 - crash in nsContentSubtreeIterator::Init

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-23c182f1-2e02-4428-999e-eff7a2120703 .
============================================================= 

Seen while looking at trunk crash stats. Small volume Mac crash - https://crash-stats.mozilla.com/report/list?signature=nsRange::GetCommonAncestor which first started showing up using 2012070203 build.

STR:
1. Load http://itunes.apple.com/us/app/track-8/id519363162?mt=8
2. Drag the scrollbar to the right to scroll the screenshots.
3. Crash

Possible regression range based on crash stats: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d9d61d199b11&tochange=a31fc9052840

Frame 	Module 	Signature 	Source
0 	XUL 	nsRange::GetCommonAncestor 	nsRange.cpp:748
1 	XUL 	nsContentSubtreeIterator::Init 	nsContentIterator.cpp:1195
2 	XUL 	mozilla::Selection::selectFrames 	nsSelection.cpp:4007
3 	XUL 	mozilla::Selection::ReplaceAnchorFocusRange 	nsSelection.cpp:4708
4 	XUL 	nsFrameSelection::HandleClick 	nsSelection.cpp:1387
5 	XUL 	nsFrameSelection::HandleDrag 	nsSelection.cpp:1500
6 	XUL 	nsFrame::HandleDrag 	nsFrame.cpp:2887
7 	XUL 	nsEventTargetChainItem::HandleEventTargetChain 	nsEventDispatcher.cpp:362
8 	XUL 	nsEventDispatcher::Dispatch 	nsEventDispatcher.cpp:639
9 	XUL 	PresShell::HandleEventInternal 	nsPresShell.cpp:6477
10 	XUL 	PresShell::HandlePositionedEvent 	nsPresShell.cpp:6208
11 	XUL 	PresShell::HandleEvent 	nsPresShell.cpp:6010
12 	XUL 	PresShell::HandleEvent 	nsPresShell.cpp:5747
13 	XUL 	nsViewManager::DispatchEvent 	nsViewManager.cpp:893
14 	XUL 	HandleEvent 	nsView.cpp:127
15 	XUL 	nsChildView::DispatchEvent 	nsChildView.mm:1486
16 	XUL 	nsChildView::DispatchWindowEvent 	nsChildView.mm:1496
17 	XUL 	-[ChildView mouseDragged:] 	nsChildView.mm:3511
18 	AppKit 	AppKit@0xd57ab 	
19 	libsystem_c.dylib 	libsystem_c.dylib@0x4d46f 	
20 	libsystem_c.dylib 	libsystem_c.dylib@0x4d6aa 	
21 	AppKit 	AppKit@0x98f8bf 	
22 	AppKit 	AppKit@0x98f8bf 	
23 	Foundation 	Foundation@0x8405 	
24 	AppKit 	AppKit@0x6bda3 	
25 	AppKit 	AppKit@0x6bda3 	
26 	AppKit 	AppKit@0x6ca73 	
27 	libobjc.A.dylib 	libobjc.A.dylib@0xb0ff 	
28 	XUL 	-[ToolbarWindow sendEvent:] 	nsCocoaWindow.mm:2721
29 	AppKit 	AppKit@0x6ea54 	
30 	libobjc.A.dylib 	libobjc.A.dylib@0xd2c5 	
31 	libobjc.A.dylib 	libobjc.A.dylib@0xd254 	
32 	CoreFoundation 	CoreFoundation@0x309a8 	
33 	AppKit 	AppKit@0x6d0df 	
34 	AppKit 	AppKit@0x8aa4 	
35 	XUL 	nsHTMLBodyElement::GetOnload 	nsEventNameList.h:354
36 	XUL 	-[GeckoNSApplication sendEvent:] 	nsAppShell.mm:157
37 	AppKit 	AppKit@0x50c5 	
38 	XUL 	nsAppShell::Run 	nsAppShell.mm:756
39 	XUL 	nsAppStartup::Run 	nsAppStartup.cpp:256
40 	XUL 	XREMain::XRE_mainRun 	nsAppRunner.cpp:3786
41 	XUL 	XREMain::XRE_main 	nsAppRunner.cpp:3863
42 	XUL 	XRE_main 	nsAppRunner.cpp:3939
43 	firefox 	main 	nsBrowserApp.cpp:160
44 	firefox 	firefox@0x14f3

Comment 1

[Marcia Knous [:marcia]]

Crashes affect 10.7 and 10.8 only so far - no 10.6 crashes.

Comment 2

[Scoobidiver (away)]

Here are Windows crashes:
https://crash-stats.mozilla.com/report/list?signature=nsContentSubtreeIterator%3A%3AInit%28nsIDOMRange*%29

Comment 3

[Scoobidiver (away)]

There's a good chance it's a regression from bug 767169.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Looks like in some cases aRange is null.  This is checked only by MOZ_ASSERT, so it will crash if it's false.  This is probably due to bug 767169, because previously selectFrames would return an error if aRange was null and this changed it to MOZ_ASSERT.  It looks like it's being called by ReplaceAnchorFocusRange, which is sometimes called when mAnchorFocusRange is null and calls selectFrames to deselect the old range.

I think the right thing to do here is to add a null check to ReplaceAnchorFocusRange.  Calling selectFrames on a null range looks bogus and we probably want to know about it.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Clarify Selection::selectFrames parameter

It's named "aFlags", which makes no sense.  The name in the .h file is "aSelect", which does make sense.  So change that and improve the comment a bit.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- Fix crash due to removed null check in Selection::selectFrames

Try: https://tbpl.mozilla.org/?tree=Try&rev=b2311999e756

There are two issues with this patch:

1) It has no test.  I reproduced the crash from comment #0 and verified that the patch fixes it, but I don't understand selection code well enough to figure out how to trigger the crash in an automated fashion.

2) I don't know if it's correct.  Prior to bug 767169, selectFrames and SetAnchorFocusToRange both would return an error in this case if mAnchorFocusRange was null, which ReplaceAnchorFocusRange would ignore, so it would silently do nothing in that case.  This restores that behavior, but using NS_ENSURE_TRUE, so it spams lots of warnings when you try to execute the test case.  What's actually happening in this case, and how should we behave?  I don't understand what the caller (nsFrameSelection::AdjustForMaintainedSelection) is trying to do -- I see no code documentation for anything related to maintaining selections, and it's not obvious from looking.  Probably we don't want a warning here; I'm guessing we want to do a check further up the call chain.  But I don't know where.

So this is good for a quick fix for the crash, but I don't know that it's a proper solution.

Comment 7

[Scoobidiver (away)]

With combined signatures, it's #6 top browser crasher over the last 3 days.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3284f32a16b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9a9bcd5e1b4

If someone wants to make sure this gets to m-c before the next nightly is built, please do -- I can't necessarily stick around to watch the tree.

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f3284f32a16b
https://hg.mozilla.org/mozilla-central/rev/e9a9bcd5e1b4

Comment 10

[(no longer active)]

(In reply to comment #9)
> https://hg.mozilla.org/mozilla-central/rev/f3284f32a16b
> https://hg.mozilla.org/mozilla-central/rev/e9a9bcd5e1b4

A nightly was triggered on this...

Comment 11

[Marcia Knous [:marcia]]

Verified fixed using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/16.0 Firefox/16.0. No crash using the reproducible steps in Comment 0.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Not sure what happened with the tracking flags in the past few changes, but I'm untracking this since it is fixed in 16.

Comment 13

[(no longer active)]

It should still be tracking+ in case it gets backed out for example, right?