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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: jruderman, Assigned: dholbert)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
x86
All
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 280008 [details]
testcase (crashes Firefox when loaded)

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

Updated

11 years ago
Whiteboard: [sg:critical?]
(Reporter)

Updated

11 years ago
Flags: blocking1.9?
(Assignee)

Updated

11 years ago
OS: Mac OS X → All
(Assignee)

Comment 1

11 years ago
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.
(Assignee)

Updated

11 years ago
Assignee: nobody → dholbert
(Assignee)

Comment 2

11 years ago
Created attachment 283268 [details] [diff] [review]
patch v1
(Assignee)

Comment 3

11 years ago
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
(Assignee)

Comment 4

11 years ago
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.)
Attachment #283268 - Attachment is obsolete: true
Attachment #283279 - Flags: review?(roc)
(Assignee)

Comment 5

11 years ago
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.
(Assignee)

Comment 7

11 years ago
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.
Attachment #283279 - Attachment is obsolete: true
Attachment #283296 - Flags: review?(roc)
Attachment #283279 - Flags: review?(roc)
(Assignee)

Comment 8

11 years ago
(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.
(Assignee)

Comment 10

11 years ago
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.
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+
(Assignee)

Comment 11

11 years ago
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+

Updated

11 years ago
Flags: in-testsuite?
(Reporter)

Comment 12

11 years ago
This bug doesn't seem to affect branch.
Group: security
Flags: wanted1.8.1.x-
(Reporter)

Comment 13

11 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCachedStyleData::GetStyleDisplay] [@ DoDeletingFrameSubtree]

Updated

7 years ago
Depends on: 654002
You need to log in before you can comment on or make changes to this bug.