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

VERIFIED FIXED in mozilla1.9alpha8

Status

()

Core
Layout
P1
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha8
crash, fixed1.8.0.15, testcase, verified1.8.1.10
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.8 -
blocking1.8.1.9 -
blocking1.8.1.10 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(5 attachments)

(Reporter)

Description

10 years ago
Created attachment 277817 [details]
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)
(Reporter)

Comment 1

10 years ago
Created attachment 277818 [details]
testcase
(Reporter)

Comment 2

10 years ago
Created attachment 277819 [details]
all-in-one testcase

All-in-one testcase, that only trunk builds can handle.
(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.

Updated

10 years ago
Flags: blocking1.9?
Whiteboard: [sg:critical?]
Compare bug 317948.
Created attachment 277851 [details] [diff] [review]
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....
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #277851 - Flags: superreview?(dbaron)
Attachment #277851 - Flags: review?(dbaron)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Crash [@ nsCSSFrameConstructor::RemoveFirstLetterFrames] with quotes, binding, position: fixed, display: -moz-box and first-letter → [FIX]Crash [@ nsCSSFrameConstructor::RemoveFirstLetterFrames] with quotes, binding, position: fixed, display: -moz-box and first-letter
Target Milestone: --- → mozilla1.9 M8
Flags: blocking1.9? → blocking1.9+
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?
Attachment #277851 - Flags: superreview?(dbaron)
Attachment #277851 - Flags: superreview+
Attachment #277851 - Flags: review?(dbaron)
Attachment #277851 - Flags: review+
Attachment #277851 - Flags: approval1.9+
... and I think that bug should be blocking1.9+.
Fixed on trunk.  I think we should fix this on branch too.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Flags: blocking1.8.1.8?
Resolution: --- → FIXED
Attachment #277851 - Flags: approval1.8.1.8?
Attachment #277851 - Flags: approval1.8.0.14?
Filed bug 398108 to assert this as needed.
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
Attachment #277851 - Flags: approval1.8.1.8?
Attachment #277851 - Flags: approval1.8.1.8+
Attachment #277851 - Flags: approval1.8.0.14?
Attachment #277851 - Flags: approval1.8.0.14+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.8?
Flags: blocking1.8.1.8+
Flags: blocking1.8.0.14?
Fixed on both branches.
Keywords: fixed1.8.0.14, fixed1.8.1.8
(Reporter)

Comment 12

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100104 Minefield/3.0a9pre
Status: RESOLVED → VERIFIED
(Reporter)

Comment 13

10 years ago
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
Created attachment 283807 [details] [diff] [review]
Additional branch fix

This is also needed to fix the crash on branch.
Attachment #283807 - Flags: superreview?(jonas)
Attachment #283807 - Flags: review?(jonas)
Attachment #283807 - Flags: approval1.8.1.9?
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.
Flags: blocking1.8.1.9?
Keywords: fixed1.8.0.14, fixed1.8.1.8
Whiteboard: [sg:critical?] → [sg:critical?] need r=jonas
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.
Attachment #283807 - Flags: superreview?(jonas)
Attachment #283807 - Flags: superreview+
Attachment #283807 - Flags: review?(jonas)
Attachment #283807 - Flags: review+
Attachment #283807 - Flags: approval1.8.1.8?
Didn't need to re-spin; next release.
Flags: blocking1.8.1.9?
Flags: blocking1.8.1.9+
Flags: blocking1.8.1.8-
Flags: blocking1.8.1.8+
Attachment #283807 - Flags: approval1.8.1.8?
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.
Flags: blocking1.8.1.9?
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.
Not going to take on the respin, will wait for the next regular security update.
Flags: blocking1.8.1.9? → blocking1.8.1.9-
Attachment #283807 - Flags: approval1.8.1.9?
Flags: blocking1.8.1.11+ → blocking1.8.1.10+
Whiteboard: [sg:critical?] need r=jonas → [sg:critical?]
Comment on attachment 283807 [details] [diff] [review]
Additional branch fix

approved for 1.8.1.10, a=dveditz for release-drivers
Attachment #283807 - Flags: approval1.8.1.9?
Attachment #283807 - Flags: approval1.8.1.10?
Attachment #283807 - Flags: approval1.8.1.10+
Attachment #283807 - Flags: approval1.8.1.9?
Attachment #283807 - Flags: approval1.8.1.9?
Checked in the additional fix.
Keywords: fixed1.8.1.10
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
Keywords: fixed1.8.1.10 → verified1.8.1.10
Group: security
Flags: blocking1.8.0.14? → blocking1.8.0.15?
Comment on attachment 277851 [details] [diff] [review]
One possibility

Minusing partial branch patch.
Attachment #277851 - Flags: approval1.8.0.14+ → approval1.8.0.14-
Flags: blocking1.8.0.15? → blocking1.8.0.15+

Comment 25

9 years ago
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.
Attachment #283807 - Flags: approval1.8.0.15+

Updated

9 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed → fixed1.8.0.15

Comment 27

8 years ago
crash test landed 
http://hg.mozilla.org/mozilla-central/rev/de7664b0ffd2
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCSSFrameConstructor::RemoveFirstLetterFrames]
You need to log in before you can comment on or make changes to this bug.