Closed Bug 395316 Opened 15 years ago Closed 15 years ago

Crash [@ nsCachedStyleData::GetStyleDisplay] [@ DoDeletingFrameSubtree] with -moz-column and float

Categories

(Core :: Layout, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: dholbert)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical?])

Crash Data

Attachments

(3 files, 3 obsolete files)

Loading the testcase makes Firefox (Mac trunk debug) crash dereferencing 0xddddddfd.
Whiteboard: [sg:critical?]
Flags: blocking1.9?
OS: Mac OS X → All
Attached file 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.
Assignee: nobody → dholbert
Attached patch patch v1 (obsolete) — Splinter Review
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
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.)
Attachment #283268 - Attachment is obsolete: true
Attachment #283279 - Flags: review?(roc)
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.
Attachment #283279 - Attachment is obsolete: true
Attachment #283296 - Flags: review?(roc)
Attachment #283279 - Flags: review?(roc)
(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.
Attachment #283296 - Attachment is obsolete: true
Attachment #283300 - Flags: review?(roc)
Attachment #283296 - Flags: review?(roc)
Attachment #283300 - Flags: superreview+
Attachment #283300 - Flags: review?(roc)
Attachment #283300 - Flags: review+
Attachment #283300 - Flags: approval1.9+
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
Closed: 15 years ago
Resolution: --- → FIXED
Flags: blocking1.9? → blocking1.9+
Flags: in-testsuite?
This bug doesn't seem to affect branch.
Group: security
Flags: wanted1.8.1.x-
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCachedStyleData::GetStyleDisplay] [@ DoDeletingFrameSubtree]
Depends on: 654002
You need to log in before you can comment on or make changes to this bug.