330 bytes, text/html
12.18 KB, text/plain
2.60 KB, patch
|Details | Diff | Splinter Review|
Created attachment 280008 [details] testcase (crashes Firefox when loaded) Loading the testcase makes Firefox (Mac trunk debug) crash dereferencing 0xddddddfd.
Created attachment 283225 [details] backtrace at crash 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.)
Status: NEW → ASSIGNED
Created attachment 283279 [details] [diff] [review] patch v2 (using IsFloatContainingBlock) 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.
Created attachment 283296 [details] [diff] [review] patch v3 (call RemoveFrame whenever parent != this) (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.
Created attachment 283300 [details] [diff] [review] patch v4 (stop looping after recursive call to RemoveFrame) > 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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: blocking1.9? → blocking1.9+
This bug doesn't seem to affect branch.
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCachedStyleData::GetStyleDisplay] [@ DoDeletingFrameSubtree]
You need to log in before you can comment on or make changes to this bug.