Last Comment Bug 770580 - crash in nsContentSubtreeIterator::Init
: crash in nsContentSubtreeIterator::Init
Status: VERIFIED FIXED
: crash, regression, reproducible, topcrash
Product: Core
Classification: Components
Component: Selection (show other bugs)
: 16 Branch
: All All
: -- critical with 1 vote (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks: 767169
  Show dependency treegraph
 
Reported: 2012-07-03 10:14 PDT by Marcia Knous [:marcia - use ni]
Modified: 2012-07-19 11:13 PDT (History)
5 users (show)
ayg: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch part 1 -- Clarify Selection::selectFrames parameter (2.70 KB, patch)
2012-07-05 04:20 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review
Patch part 2 -- Fix crash due to removed null check in Selection::selectFrames (915 bytes, patch)
2012-07-05 04:26 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Review

Description Marcia Knous [:marcia - use ni] 2012-07-03 10:14:29 PDT
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 - use ni] 2012-07-03 11:31:48 PDT
Crashes affect 10.7 and 10.8 only so far - no 10.6 crashes.
Comment 3 Scoobidiver (away) 2012-07-04 12:54:54 PDT
There's a good chance it's a regression from bug 767169.
Comment 4 :Aryeh Gregor (away until August 15) 2012-07-05 01:11:49 PDT
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 (away until August 15) 2012-07-05 04:20:31 PDT
Created attachment 639284 [details] [diff] [review]
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 (away until August 15) 2012-07-05 04:26:09 PDT
Created attachment 639285 [details] [diff] [review]
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) 2012-07-05 10:04:48 PDT
With combined signatures, it's #6 top browser crasher over the last 3 days.
Comment 8 :Aryeh Gregor (away until August 15) 2012-07-06 01:01:45 PDT
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 10 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-06 09:49:09 PDT
(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 - use ni] 2012-07-09 09:45:07 PDT
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 2012-07-18 17:43:16 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-19 11:13:40 PDT
It should still be tracking+ in case it gets backed out for example, right?

Note You need to log in before you can comment on or make changes to this bug.