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...
Status: VERIFIED FIXED
[sg:critical?]
: 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] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-22 17:31 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
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+
caillon: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description Martijn Wargers [:mwargers] (not working for Mozilla) 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.

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 [:mwargers] (not working for Mozilla) 2007-08-22 17:33:03 PDT
Created attachment 277818 [details]
testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 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 Boris Zbarsky [:bz] (Out June 25-July 6) 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 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-08-22 22:11:08 PDT
Compare bug 317948.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 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 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-09-28 15:51:17 PDT
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-7 (review requests must explain patch) 2007-09-28 15:51:46 PDT
... and I think that bug should be blocking1.9+.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-09-30 20:15:59 PDT
Fixed on trunk.  I think we should fix this on branch too.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-09-30 20:30:26 PDT
Filed bug 398108 to assert this as needed.
Comment 10 Daniel Veditz [:dveditz] 2007-10-01 14:40:13 PDT
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 [:bz] (Out June 25-July 6) 2007-10-01 22:14:44 PDT
Fixed on both branches.
Comment 12 Martijn Wargers [:mwargers] (not working for Mozilla) 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 Martijn Wargers [:mwargers] (not working for Mozilla) 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: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 [:bz] (Out June 25-July 6) 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 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 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-10-12 09:49:48 PDT
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] 2007-10-19 11:11:22 PDT
Didn't need to re-spin; next release.
Comment 18 Daniel Veditz [:dveditz] 2007-10-21 16:13:55 PDT
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 [:bz] (Out June 25-July 6) 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 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 Daniel Veditz [:dveditz] 2007-11-07 14:12:23 PST
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 [:bz] (Out June 25-July 6) 2007-11-09 23:44:57 PST
Checked in the additional fix.
Comment 23 Carsten Book [:Tomcat] 2007-11-14 18:31:06 PST
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] 2007-12-03 15:44:57 PST
Comment on attachment 277851 [details] [diff] [review]
One possibility

Minusing partial branch patch.
Comment 25 Alexander Sack 2008-02-28 07:44:54 PST
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] (use needinfo?) 2008-03-14 09:29:06 PDT
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:] 2009-04-24 10:37:22 PDT
crash test landed 
http://hg.mozilla.org/mozilla-central/rev/de7664b0ffd2

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