Last Comment Bug 393326 - [FIX]Crash [@ nsCSSFrameConstructor::RemoveFirstLetterFrames] with quotes, binding, position: fixed, display: -moz-box and first-letter
: [FIX]Crash [@ nsCSSFrameConstructor::RemoveFirstLetterFrames] with quotes, bi...
: crash, fixed1.8.0.15, testcase, verified1.8.1.10
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
P1 critical (vote)
: mozilla1.9alpha8
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Jet Villegas (:jet)
Depends on:
  Show dependency treegraph
Reported: 2007-08-22 17:31 PDT by Martijn Wargers [:mwargers]
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
dbaron: blocking1.9+
dveditz: blocking1.8.1.8-
dveditz: blocking1.8.1.9-
dveditz: blocking1.8.1.10+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

binding needed for testcase (115 bytes, text/xml)
2007-08-22 17:31 PDT, Martijn Wargers [:mwargers]
no flags Details
testcase (261 bytes, text/html)
2007-08-22 17:33 PDT, Martijn Wargers [:mwargers]
no flags Details
all-in-one testcase (401 bytes, text/html)
2007-08-22 17:34 PDT, Martijn Wargers [:mwargers]
no flags Details
One possibility (1.66 KB, patch)
2007-08-22 22:25 PDT, Boris Zbarsky [:bz] (still a bit busy)
dbaron: review+
dbaron: superreview+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.14-
dbaron: approval1.9+
Details | Diff | Splinter Review
Additional branch fix (1.07 KB, patch)
2007-10-05 23:07 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
jonas: superreview+
dveditz: approval1.8.1.10+
Details | Diff | Splinter Review

Description User image Martijn Wargers [:mwargers] 2007-08-22 17:31:48 PDT
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.
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

For branch Talkback ID: TB35197028E (stacktrace looks sort of similar)
Comment 1 User image Martijn Wargers [:mwargers] 2007-08-22 17:33:03 PDT
Created attachment 277818 [details]
Comment 2 User image Martijn Wargers [:mwargers] 2007-08-22 17:34:50 PDT
Created attachment 277819 [details]
all-in-one testcase

All-in-one testcase, that only trunk builds can handle.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-22 22:08:56 PDT
(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 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-22 22:11:08 PDT
Compare bug 317948.
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2007-08-22 22:25:58 PDT
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....
Comment 6 User image David Baron :dbaron: ⌚️UTC-8 2007-09-28 15:51:17 PDT
Comment on attachment 277851 [details] [diff] [review]
One possibility


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 User image David Baron :dbaron: ⌚️UTC-8 2007-09-28 15:51:46 PDT
... and I think that bug should be blocking1.9+.
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2007-09-30 20:15:59 PDT
Fixed on trunk.  I think we should fix this on branch too.
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2007-09-30 20:30:26 PDT
Filed bug 398108 to assert this as needed.
Comment 10 User image Daniel Veditz [:dveditz] 2007-10-01 14:40:13 PDT
Comment on attachment 277851 [details] [diff] [review]
One possibility

approved for and, a=dveditz for release-drivers
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-01 22:14:44 PDT
Fixed on both branches.
Comment 12 User image Martijn Wargers [:mwargers] 2007-10-02 03:24:06 PDT
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100104 Minefield/3.0a9pre
Comment 13 User image Martijn Wargers [:mwargers] 2007-10-04 15:07:22 PDT
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: Gecko/20071004 BonEcho/

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]
Comment 14 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-05 23:07:39 PDT
Created attachment 283807 [details] [diff] [review]
Additional branch fix

This is also needed to fix the crash on branch.
Comment 15 User image Daniel Veditz [:dveditz] 2007-10-12 09:26:51 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-12 09:49:48 PDT
Sorry about that.  I was assuming that was more done than it was apparently, and didn't want to spam you with unecessary approval requests.
Comment 17 User image Daniel Veditz [:dveditz] 2007-10-19 11:11:22 PDT
Didn't need to re-spin; next release.
Comment 18 User image Daniel Veditz [:dveditz] 2007-10-21 16:13:55 PDT
If we firedrill to fix layout regressions in do we want to include this? Seems a shame not to since it's tested and only missed 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 User image Boris Zbarsky [:bz] (still a bit busy) 2007-10-21 17:15:39 PDT
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 User image Daniel Veditz [:dveditz] 2007-10-22 14:43:13 PDT
Not going to take on the respin, will wait for the next regular security update.
Comment 21 User image Daniel Veditz [:dveditz] 2007-11-07 14:12:23 PST
Comment on attachment 283807 [details] [diff] [review]
Additional branch fix

approved for, a=dveditz for release-drivers
Comment 22 User image Boris Zbarsky [:bz] (still a bit busy) 2007-11-09 23:44:57 PST
Checked in the additional fix.
Comment 23 User image Carsten Book [:Tomcat] 2007-11-14 18:31:06 PST
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2007111404 BonEcho/ - no crash on testcase

Adding verified keyword
Comment 24 User image Daniel Veditz [:dveditz] 2007-12-03 15:44:57 PST
Comment on attachment 277851 [details] [diff] [review]
One possibility

Minusing partial branch patch.
Comment 25 User image Alexander Sack 2008-02-28 07:44:54 PST
Comment on attachment 283807 [details] [diff] [review]
Additional branch fix

a=asac for

attachment 277851 [details] [diff] [review] is already in the branch for whatever reason. so this should be enough.
Comment 26 User image Reed Loden [:reed] (use needinfo?) 2008-03-14 09:29:06 PDT

Checking in content/xbl/src/nsXBLService.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v  <--  nsXBLService.cpp
new revision:; previous revision:
Comment 27 User image Bob Clary [:bc:] 2009-04-24 10:37:22 PDT
crash test landed

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