Loading the testcase makes Firefox (Mac trunk debug) crash dereferencing 0xddddddfd.

Here's my backtrace at the crash. At level #2, in nsPlaceholderFrame::CanContinueTextRun, we're hitting this line: 159 return mOutOfFlowFrame->CanContinueTextRun(); At that point, mOutOfFlowFrame points to a nsIFrame whose pointers are all 0xdddddddd.

The entirety of this bug's problems happen inside of nsFrameConstructor::ContentRemoved, between lines 9521 and 9538. (Link: http://mxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#9500 ) -- First, we unregister the placeholders via the call to UnregisterPlaceholderChain -- HOWEVER, this function doesn't clear the placeholders' mOutOfFlowFrame pointers, and that's what was causing this bug's crash. The first hunk of my patch fixes that. (making the behavior there match nsBlockFrame::RemoveFloat, at http://mxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#4919 ) -- After that's fixed, though, we're open to another crash. This call in nsCSSFrameConstructor: 9529 rv = frameManager->RemoveFrame(parentFrame, ... 9531 childFrame); correctly removes the floated span *and* its continuations, but the call at line 9537 9537 rv |= frameManager->RemoveFrame(placeholderParent, 9538 nsnull, placeholderFrame); only removes the *first* placeholder, leaving the other placeholders dangling. My patch's second chunk fixes this. (The subsequent placeholders didn't get removed previously because their parents were nsBlockFrames, and so they were in their parents mLines rather than on the mFrames list, and so the original "parent->mFrames.DestroyFrame()" call did nothing.)

Same as prev patch, but uses IsFloatContainingBlock so that special case in nsContainerFrame will work on all subclasses of nsBlockFrame. (instead of only working on those that satisfy GetType() == blockFrame, as in the first patch.)

Comment on attachment 283279 [details] [diff] [review] patch v2 (using IsFloatContainingBlock) >+ // NOTE: It's important that, for any frames that have >+ // IsFloatContainingBlock() == true, we have a *different* >+ // implementation of RemoveFrame from this one that we're currently >+ // inside. Otherwise, we could get into an endless recursive loop of >+ // nsContainerFrame::RemoveFrame calls here. >+ parent->RemoveFrame(nsnull, aOldFrame); Note: That's just a precautionary comment. We're safe in that department right now, because nsBlockFrame (the only thing for which IsFloatContainingBlock() == true) has a custom implementation of RemoveFrame.

Why don't we just call parent->mFrames.DestroyFrame as long as parent == this, and then when parent becomes != this, call parent->RemoveFrame unconditionally? That would prevent infinite recursion and be correct in all cases. Plus, after we've called RemoveFrame once we need to stop since all the continuations will then have been destroyed.

(In reply to comment #6) > Why don't we just call parent->mFrames.DestroyFrame as long as parent == this, > and then when parent becomes != this, call parent->RemoveFrame unconditionally? I like it. Done.

(btw -- I tested it again to make sure, and patch v3 fixes the testcase, and it passes reftests.)

What about this last comment? > Plus, after > we've called RemoveFrame once we need to stop since all the continuations will > then have been destroyed.

> What about this last comment? Oops, I glanced over that part too quickly before. This patch throws in a break and a comment to address that.

patch v4 checked in: Checking in layout/base/nsCSSFrameConstructor.cpp; /cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v <-- nsCSSFrameConstructor.cpp new revision: 1.1406; previous revision: 1.1405 done Checking in layout/generic/nsContainerFrame.cpp; /cvsroot/mozilla/layout/generic/nsContainerFrame.cpp,v <-- nsContainerFrame.cpp new revision: 1.290; previous revision: 1.289 done

This bug doesn't seem to affect branch.

Crashtest checked in.