Debug: Assertion failure: !GetPropTableFrames(aPresContext, aProperty), at layout/generic/nsContainerFrame.cpp:1459 ASan: Use-after-poison of pres arena memory in nsFrameList::UnhookFrameFromSiblings. Nightly: No symptoms.

Security-sensitive in case this is a long-standing bug revealed by bug 729519.

The check for when to forget about mOverflowContList is wrong. http://hg.mozilla.org/mozilla-central/annotate/4420f27742c7/layout/generic/nsContainerFrame.cpp#l1770 Here's the stack / frame tree for the call to Finish() when we shouldn't forget about it, because only the first two children (ColumnSet frames) will be removed. It seems we also update mSentry/mPrevOverflowCont wrong - if I fix the above bug, then the resulting mSentry/mPrevOverflowCont points to a frame (red) that is going to be destroyed. I think mSentry should point to the green block, and mPrevOverflowCont its prev-in-flow (pink), but I'm not really sure...

I'm pretty sure this is mitigated by frame poisoning. After nsOverflowContinuationTracker::Insert creates a new list and overwrites the ExcessOverflowContainers property (the assertion in comment 0) there's no way to access the old list anymore. The crash is either going to be accessing a destroyed frame through the bogus mSentry/mPrevOverflowCont or when DeleteNextInFlowChild is trying to unhook one of the frames and finds a next-sibling that is destroyed because we failed to remove it from the new frame list earlier so it's still hooked up (the ASan error above).

BEFORE: mSentry=0x7f4fe5b03330 mPrevOverflowCont=(nil) AFTER: mSentry=0x7f4fe5b03af0 mPrevOverflowCont=(nil) fantasai, does this makes sense to you for the given frame trees?

Call BeginFinish() before DeleteNextInFlowChild/StealFrame on the kid's next-in-flow and EndFinish() after, with the help of the new AutoFinish class. The old Finish() method tried to figure out whether the OC frame list would become empty, and thus deleted. This is too fragile and never really worked. Instead, compare the list pointer with mParent's OC lists after the kid's next-in-flows are gone, in EndFinish(), and re-setup the tracker's list if it was deleted. Break out the list setup code from the ctor to a new method, SetupOverflowContList(), so that it can be used for that purpose. In BeginFinish, only try to detect if mSentry/mPrevOverflowCont are going stale and repair those later in EndFinish(). (to my surprise "f == mPrevOverflowCont" actually do occur, which seems to contradict the documentation that "it's a known good point; this pointer won't be deleted on us") I'm assuming that if we bump into mSentry without seeing mPrevOverflowCont then mPrevOverflowCont is still valid - does this assumption hold? It would be great if you could elaborate on what mPrevOverflowCont/ mSentry is used for, and the relation between them. If one is null, what does that mean? and what values can the other have in that case? https://tbpl.mozilla.org/?tree=Try&rev=c3cebb6cf160

WFM on mozilla-central??

The first good revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/52127bafd50b user: Scott Johnson date: Wed Apr 24 10:02:36 2013 -0500 summary: Bug 857324: Make column set reflow continue without balancing rather than restarting if computed height is exceeded. [r=mats]

Comment on attachment 740090 [details] [diff] [review] fix I still think we should fix this.

Comment on attachment 740090 [details] [diff] [review] fix Review of attachment 740090 [details] [diff] [review]: ----------------------------------------------------------------- I'm rubber-stamping this.

(In reply to Jesse Ruderman from comment #1) > Security-sensitive in case this is a long-standing bug revealed by bug > 729519. This means 22 and 23 are affected by this and would, ideally, have the fix ported to them, right?

Bug 729519 was fixed in 22 so this crash is mitigated by arena poisoning from that version, so it's not necessary to uplift this IMO.

Same for 23?

Yes, 22 and all later versions.

Landed a crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e37220caed