393326 - [FIX]Crash [@ nsCSSFrameConstructor::RemoveFirstLetterFrames] with quotes, binding, position: fixed, display: -moz-box and first-letter

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: binding needed for testcase

See upcoming testcase, which crashes on current trunk and branch builds.
Because it's crashing on branch, I'm marking it security sensitive for now.

http://crash-stats.mozilla.com/report/index/8813af68-5107-11dc-a661-001a4bd43e5c
0  	nsCSSFrameConstructor::RemoveFirstLetterFrames(nsPresContext*, nsIPresShell*, nsFrameManager*, nsIFrame*, int*)  	 nsCSSFrameConstructor.cpp:1.1394:12041
1 	nsCSSFrameConstructor::RemoveLetterFrames(nsPresContext*, nsIPresShell*, nsFrameManager*, nsIFrame*) 	nsCSSFrameConstructor.cpp:1.1394:12102
2 	nsCSSFrameConstructor::CharacterDataChanged(nsIContent*, int) 	nsCSSFrameConstructor.cpp:1.1394:9774
3 	PresShell::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*) 	nsPresShell.cpp:3.1051:4454
4 	nsBindingManager::CharacterDataChanged(nsIDocument*, nsIContent*, CharacterDataChangeInfo*) 	nsBindingManager.cpp:1.184:1190
5 	nsNodeUtils::CharacterDataChanged(nsIContent*, CharacterDataChangeInfo*) 	nsNodeUtils.cpp:3.34:88
6 	nsGenericDOMDataNode::SetTextInternal(unsigned int, unsigned int, unsigned short const*, unsigned int, int) 	nsGenericDOMDataNode.cpp:3.240:483
7 	nsGenericDOMDataNode::SetNodeValue(nsAString_internal const&) 	nsGenericDOMDataNode.cpp:3.240:124
8 	nsCommentNode::SetNodeValue(nsAString_internal const&) 	nsXMLProcessingInstruction.cpp:1.75:162
9 	nsQuoteList::RecalcAll() 	nsQuoteList.cpp:1.6:95
10 	nsCSSFrameConstructor::NotifyDestroyingFrame(nsIFrame*) 	nsCSSFrameConstructor.cpp:1.1394:1854
11 	PresShell::NotifyDestroyingFrame(nsIFrame*) 	nsPresShell.cpp:3.1051:2570
12 	nsFrame::Destroy() 	nsFrame.cpp:3.747:641
13 	nsContainerFrame::Destroy() 	nsContainerFrame.cpp:1.288:294
14 	nsFrameList::DestroyFrames() 	nsFrameList.cpp:3.48:67
15 	nsContainerFrame::Destroy() 	nsContainerFrame.cpp:1.288:252
16 	nsLineBox::DeleteLineList(nsPresContext*, nsLineList&) 	nsLineBox.cpp:1.121:363
17 	nsBlockFrame::Destroy() 	nsBlockFrame.cpp:3.860:300
18 	nsFrameList::DestroyFrame(nsIFrame*) 	nsFrameList.cpp:3.48:162
19 	nsAbsoluteContainingBlock::RemoveFrame(nsIFrame*, nsIAtom*, nsIFrame*) 	nsAbsoluteContainingBlock.cpp:1.91:127
20 	ViewportFrame::RemoveFrame(nsIAtom*, nsIFrame*) 	nsViewportFrame.cpp:1.98:156
etc.

For branch Talkback ID: TB35197028E (stacktrace looks sort of similar)

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: all-in-one testcase

All-in-one testcase, that only trunk builds can handle.

Comment 3

[Boris Zbarsky [:bzbarsky]]

(gdb) frame
#0  0xb4b19cb6 in nsCSSFrameConstructor::FindFrameWithContent (this=0x8ba9e88, 
    aFrameManager=0x8a23bcc, aParentFrame=0xafe15b38, aParentContent=0x8bb3720, 
    aContent=0x8bb3818, aHint=0x0)
    at ../../../mozilla/layout/base/nsCSSFrameConstructor.cpp:10624
10624             if (aParentContent == kidContent ||
10625                 (aParentContent && (aParentContent == kidContent->GetBindingParent()))) 
(gdb) p kidContent
$2 = (Cannot access memory at address 0xdddddddd

In this case, aParentFrame is an out-of-flow that is no longer in the frame tree, but still has a placeholder pointing to it.

The issue is that we're under nsAbsoluteContainingBlock::RemoveFrame (called from a frame reconstruct via nsCSSFrameConstructor::ProcessRestyledFrames called from PresShell::RecreateFramesFor (called by XBL).  When we destroy the frame we end up in PresShell::NotifyDestroyingFrame which calls nsCSSFrameConstructor::NotifyDestroyingFrame which dirties the quotes list.  But we're not in an update batch, so QuotesDirty does a RecalcAll, which sets text on the text node for the quote, which calls into nsCSSFrameConstructor::CharacterDataChanged, which calls GetPrimaryFrameFor()... and then we die.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Compare bug 317948.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: One possibility

This fixes the bug, but maybe we should begin/end updates wherever we enter/exit the frame constructor phase?  That would make sure that we never update quotes while actually in the middle of frame construction....

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 277851 [details] [diff] [review]
One possibility

r+sr+a1.9=dbaron.

But please at least file a bug on enforcing the invariants that we really want to enforce here -- e.g., should we be asserting about the update count in certain places?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... and I think that bug should be blocking1.9+.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  I think we should fix this on branch too.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Filed bug 398108 to assert this as needed.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 277851 [details] [diff] [review]
One possibility

approved for 1.8.1.8 and 1.8.0.14, a=dveditz for release-drivers

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed on both branches.

Comment 12

[Martijn Wargers (dead)]

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

Comment 13

[Martijn Wargers (dead)]

I'm still crashing on the 1.8 branch with the testcase, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071004 BonEcho/2.0.0.8pre

Talkback ID: TB36609315G
nsCSSFrameConstructor::RemoveFirstLetterFrames  [mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13065]
nsCSSFrameConstructor::RemoveLetterFrames  [mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13129]
nsCSSFrameConstructor::CharacterDataChanged  [mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10420]
PresShell::CharacterDataChanged  [mozilla/layout/base/nsPresShell.cpp, line 5486]
nsGenericDOMDataNode::DoSetText  [mozilla/content/base/src/nsGenericDOMDataNode.cpp, line 1292]
nsGenericDOMDataNode::SetText  [mozilla/content/base/src/nsGenericDOMDataNode.h, line 256]
nsXMLCDATASection::SetData  [mozilla/content/xml/content/src/nsXMLCDATASection.cpp, line 59]
nsHTMLFormElement::AddRef
0xf9b0e918

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional branch fix

This is also needed to fix the crash on branch.

Comment 15

[Daniel Veditz [:dveditz]]

argh! We had time to get this into the rc2 respin if the flags had been managed better.

Next time please "reopen" a bug that fails branch verification (by removing the fixed flag). If it's definitely a new issue (this one isn't) you can file a new branch bug but please make sure that one is nominated for all the same branch flags.

And if you've got a patch for a "blocking1.8.1.8" bug please ask for approval on that release. Even if it's late, it gives us a fighting chance to make sure we get all the patches we need into any respin.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Sorry about that.  I was assuming that 1.8.1.8 was more done than it was apparently, and didn't want to spam you with unecessary approval requests.

Comment 17

[Daniel Veditz [:dveditz]]

Didn't need to re-spin; next release.

Comment 18

[Daniel Veditz [:dveditz]]

If we firedrill to fix layout regressions in 2.0.0.8 do we want to include this? Seems a shame not to since it's tested and only missed 2.0.0.8 because it fell off the radar (partially fixed). On the other hand including a security fix might draw undue attention to what is a fairly minor potential vulnerability.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I think that the "additional patch" is pretty opaque in terms of how it relates to the crash here.  So if the worry is that people will learn how to exploit it, that's not an issue.

If the worry is that people will overstate the severity of the vulnerability (and in particular assume that it triggered the firedrill), then I agree that could happen.  Not sure we should care, though.

Comment 20

[Daniel Veditz [:dveditz]]

Not going to take on the respin, will wait for the next regular security update.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 283807 [details] [diff] [review]
Additional branch fix

approved for 1.8.1.10, a=dveditz for release-drivers

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in the additional fix.

Comment 23

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.10 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.10pre) Gecko/2007111404 BonEcho/2.0.0.10pre - no crash on testcase

Adding verified keyword

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 277851 [details] [diff] [review]
One possibility

Minusing partial branch patch.

Comment 25

[Alexander Sack]

Comment on attachment 283807 [details] [diff] [review]
Additional branch fix

a=asac for 1.8.0.15

attachment 277851 [details] [diff] [review] is already in the branch for whatever reason. so this should be enough.

Comment 26

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in content/xbl/src/nsXBLService.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v  <--  nsXBLService.cpp
new revision: 1.204.4.1.4.1; previous revision: 1.204.4.1
done

Comment 27

[Bob Clary [:bc] (inactive)]

crash test landed 
http://hg.mozilla.org/mozilla-central/rev/de7664b0ffd2