115 bytes, text/xml
261 bytes, text/html
401 bytes, text/html
1.66 KB, patch
|Details | Diff | Splinter Review|
1.07 KB, patch
Alexander Sack: approval1.8.0.next+
|Details | Diff | Splinter Review|
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)
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.
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....
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?
... and I think that bug should be blocking1.9+.
Fixed on trunk. I think we should fix this on branch too.
Filed bug 398108 to assert this as needed.
Comment on attachment 277851 [details] [diff] [review] One possibility approved for 188.8.131.52 and 184.108.40.206, a=dveditz for release-drivers
Fixed on both branches.
Verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100104 Minefield/3.0a9pre
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:220.127.116.11pre) Gecko/20071004 BonEcho/18.104.22.168pre 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.
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 "blocking22.214.171.124" 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.
Sorry about that. I was assuming that 126.96.36.199 was more done than it was apparently, and didn't want to spam you with unecessary approval requests.
Didn't need to re-spin; next release.
If we firedrill to fix layout regressions in 188.8.131.52 do we want to include this? Seems a shame not to since it's tested and only missed 184.108.40.206 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.
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.
Comment on attachment 283807 [details] [diff] [review] Additional branch fix approved for 220.127.116.11, a=dveditz for release-drivers
Checked in the additional fix.
verified fixed 18.104.22.168 using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:22.214.171.124pre) Gecko/2007111404 BonEcho/126.96.36.199pre - no crash on testcase Adding verified keyword
Comment on attachment 277851 [details] [diff] [review] One possibility Minusing partial branch patch.
10 years ago
Comment on attachment 283807 [details] [diff] [review] Additional branch fix a=asac for 188.8.131.52 attachment 277851 [details] [diff] [review] is already in the branch for whatever reason. so this should be enough.
MOZILLA_1_8_0_BRANCH: Checking in content/xbl/src/nsXBLService.cpp; /cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v <-- nsXBLService.cpp new revision: 184.108.40.206.4.1; previous revision: 220.127.116.11 done
crash test landed http://hg.mozilla.org/mozilla-central/rev/de7664b0ffd2