Firefox crashes due to touching memory with a random address, so I'm filing this bug as security-sensitive. Here's the top of a stack trace from an opt build: Exception: EXC_BAD_ACCESS (0x0001) Codes: KERN_INVALID_ADDRESS (0x0001) at 0x48000084 <-- random Thread 0 Crashed: 0 nsCounterList::SetScope 1 nsCounterManager::AddResetOrIncrement 2 nsCounterManager::AddCounterResetsAndIncrements 3 nsCSSFrameConstructor::InitAndRestoreFrame 4 nsCSSFrameConstructor::ConstructBlock

While processing restyles we batch changes to counter lists: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1296&root=/cvsroot&mark=12914,12920,12927#12885 When a frame is destroyed it's notified to nsCounterManager::DestroyNodesFor which removes all nodes referring to the frame, but it leaves all the mScope* pointers of the nodes intact and marks the list "dirty". (I think this is a regression from bug 317948.) The crash occurs when accesing a dangling mScope* pointer (in SetScope in this case). We need to call RecalcAll() for the counter list before that happens.

This patch delayes the RecalcAll() for as long as possible. These methods depends on the mScope* pointers: nsCounterList::SetScope nsCounterList::ValueBefore (only called from nsCounterNode::Calc() which should only be used when the list is not dirty) nsCounterUseNode::GetText - which should only be called after it's been inserted into the counter list, which indirectly calls SetScope(). So, doing a RecalcAll() in SetScope() if the list is dirty should be enough. A bit fragile perhaps. An alternative is to do it directly for each frame destroyed: nsCounterList::DestroyNodesFor(nsIFrame* aFrame) { if (nsGenConList::DestroyNodesFor(aFrame)) RecalcAll(); }

The relevant code in nsCSSFrameConstructor/nsCounterManager is almost exactly the same on the 1.8 branch and the bug should occur there too, but I haven't been able to crash a branch build so far (and I assume Jesse didn't either). Odd.

Wouldn't a better fix be to not call SetScope() in nsCounterList::Insert() and not to call Calc() in nsCounterManager::AddResetOrIncrement if the list is dirty? That would ensure that we don't end up doing the frametree-touching that RecalcAll does while we're reconstructing frames.

For what it's worth, I'd be happy to write up that patch, but I can't reproduce the original crash, which would make it kinda hard to test the patch... Oh, and SetScope() and Calc() could assert that the list is not dirty...

(In reply to comment #4) > Wouldn't a better fix be to not call SetScope() in nsCounterList::Insert() > not to call Calc() in nsCounterManager::AddResetOrIncrement if the list is > dirty? That would ensure that we don't end up doing the frametree-touching > that RecalcAll does while we're reconstructing frames. Yes, but we still need to guarantee that Calc() is called before someone tries to get the value with GetText(). I don't think this solvable in general the way nsCSSFrameConstructor is written: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1296&root=/cvsroot&mark=2026,2035#2011 Imagine the counter list is already dirty before we Insert the node... We could make it work if could divide "frame construction" into a "destroy phase" (after which we do ReCalcAll) and a "build phase", and then make sure we only reach CreateGeneratedFrameFor() from the "build phase". This seems hard though. It's much easier to make the counter list resilient to frame destruction. This code needs to be cleaned up anyway IMHO. I don't like the way it's organized at the moment where all methods+data are public and the knowledge about which order to call stuff, which methods can be called while dirty etc has to be known by the caller. This is better handled by the objects themselves.

* make "dirty list" an internal state that the caller does not need to worry about * when a node is Inserted, if the list is non-dirty and the node is last, then recalculate the node data directly and leave the list non-dirty, as before. Otherwise mark it dirty. * ReCalcAll only when we need a node value (and the list is dirty) * make nsCounterList::ReCalcAll deal with the fact that some pointers could be dangling * make all internal methods+data non-public * remove code in nsCSSFrameConstructor/nsPresShell that was only needed to track counter/quote dirtiness

Comment on attachment 252011 [details] [diff] [review] Patch rev. 2 > I don't think this solvable in general the way nsCSSFrameConstructor is > written That code is bogus anyway. Again, it could be solved by making that chunk of code more like: counterList->Insert(node); PRBool dirty = counterList->IsDirty(); if (!dirty) { if (counterList->IsLast(node)) { node->Calc(counterList); node->GetText(contentString); } else { counterList->SetDirty(); CountersDirty(); dirty = PR_TRUE; } } textPtr = &node->mText; // text node assigned below This patch won't work for several reasons: 1) When counter stuff changes we actually need to change it. Right now that happens because either we Calc() nodes as we add them and then set the right text on the generated content up front, or because we go through and RecalcAll() as soon as the update is over; that sets new text on all the generated content and triggers the corresponding reflows and paints. With this patch , this doesn't happen anymore, unless someone calls GetTextFor()... which generally no one will if we, say, remove a counter node from the list. I do wonder how much you tested this with actual document mutations... 2) The whole point of the dirty thing is to NOT recalc lots of times on batched changes. Doing recalc on misplaced GetTextFor() calls instead of eliminating said calls is not the way to achieve that.

I'm thinking something more like this. We could also make Calc() bail if the list is dirty, but the frame constructor code needs to check anyway, as you noticed, because it wants to call GetText(). I do agree that it might make sense to replace the frame constructor code in question with an nsCounterManager method that takes a PRUnichar* and outputs an nsCOMPtr<nsIDOMCharacterData>* and an nsString, returning true/false the way AddResetOrIncrement() does to indicate to the caller whether RecalcAll() should be called. Or rather, return nsresult so we can still handle OOM, and have three out params. I can do that if you want. Oh, and the fourth bullet from comment 7 is not an issue, since recalculating the whole list sets the mScope pointers on the first node directly, and then only uses the information from previous nodes to set the mScope pointers on a given node. So it's safe to call no matter what the pointers look like.

(In reply to comment #8) Oh, I didn't quite get that it was RecalcAll() that flushed out the new text values... so, if I had kept the Begin/EndUpdate() stuff the patch would be mostly ok I think ;)

(In reply to comment #9) > I can do that if you want. Not necessary, I think your patch is fine if we just want to fix the crash with minimal changes. > So it's safe to call no matter what the pointers look like. Ok, I see that now.

Comment on attachment 252013 [details] [diff] [review] More like this r=mats

For branch we definitely want minimal changes. ;) I do think we should write some mutation correctness tests for this stuff...

This is coming in late for 1.8.1.2, but the patch looks small enough to hope for quick reviews and trunk testing that we'll try to take it.

Comment on attachment 252013 [details] [diff] [review] More like this sr=dbaron

Fixed.

Comment on attachment 252013 [details] [diff] [review] More like this This makes sure we don't work with possibly-dangling pointers. I think it should be pretty safe...

Comment on attachment 252013 [details] [diff] [review] More like this Approved for both branches, a=jay for drivers.

Fixed on branches.

v.fixed on both branches with 1.5.0.10 and 2.0.0.2 RC1 builds. No crash with Jesse's testcase in comment #0, reloaded about 30+ times with no hang or crash.

Crashtest checked in.