Closed
Bug 588158
Opened 15 years ago
Closed 15 years ago
Crash [@ nsLineBox::IndexOf] with -moz-column, float
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla2.0b5
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: jruderman, Assigned: dbaron)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression, testcase, Whiteboard: [sg:dos (critical w/out frame-poisoning)])
Crash Data
Attachments
(4 files, 1 obsolete file)
No description provided.
Reporter | ||
Comment 1•15 years ago
|
||
Assignee | ||
Comment 2•15 years ago
|
||
This is rather a scary stack. I'm still thinking about what we ought to do about it.
Assignee | ||
Comment 3•15 years ago
|
||
Actually, I think this is a problem that should already be taken care of by the nsLayoutUtils::IsProperAncestorFrame check in nsPlaceholderFrame::DestroyFrom . The question is why it's not.
Assignee | ||
Comment 4•15 years ago
|
||
Oh, except that we skip that check for floats, and I don't know why.
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 5•15 years ago
|
||
So in the normal case (no pagination) with floats, this check doesn't matter, since we destroy a block's float list before we destroy a block's in-flow children. This means that the placeholder's pointer to the out-of-flow will already be destroyed by the code in nsFrame::DestroyFrom (the if (mState & NS_FRAME_OUT_OF_FLOW) code).
However, bug 563584 introduced cases where the out-of-flow is in the float list of a later continuation, which are therefore cases where the out-of-flow would be destroyed after the placeholder -- exactly the cases for which we need this check.
So I think the float check should be removed.
I haven't yet investigated why it was there in the first place; I'll do that in a bit.
Assignee | ||
Updated•15 years ago
|
blocking2.0: --- → final+
Assignee | ||
Comment 6•15 years ago
|
||
The current code was introduced in bug 508473, patch 3. I can't find directly corresponding code removed in patch 2; however, I can find this comment that was removed:
- // Don't SetOutOfFlowFrame(nsnull) here because the float cache depends
- // on it when the float is removed later on, see bug 348688 comment 6.
although the old way of doing this was a bit different since DeletingFrameSubtree was a pass that ran before frame destruction.
However, the comment referenced suggests that the original reason no longer applies, since http://hg.mozilla.org/mozilla-central/rev/9d788a1b2364 changed the float cache to contain a pointer to the frame instead of a pointer to the placeholder.
I'm not sure if that comment is why the check was added in bug 508473, though.
So I'm still inclined to remove the check.
Assignee | ||
Comment 7•15 years ago
|
||
Yes, this patch does fix it.
I'd be interested if fantasai remembers why this code was here, though.
Attachment #467211 -
Flags: review?(roc)
Attachment #467211 -
Flags: feedback?(fantasai.bugs)
Assignee | ||
Comment 8•15 years ago
|
||
Also, does this bug really need to stay security-sensitive? It appears to be mitigated by frame poisoning, and as far as I can tell it affects only our trunk / beta users, who all have frame poisoning.
Reporter | ||
Comment 9•15 years ago
|
||
In my stack it seems to be mitigated by frame poisoning, but in the valgrind stack it doesn't. Do you know why?
Comment 10•15 years ago
|
||
Comment on attachment 467211 [details] [diff] [review]
patch
I actually have a very similar patch in my patch queue -- I noticed the problem when I was porting the frame destruction fix (bug 508473) back to 1.9.2 last week, but didn't know what symptoms it would cause and hadn't gotten around to filing a bug report this week. There was an error in updating the patch to use the placeholder bit checks -- it's supposed to check for popup frames. The patch there was originally
+ (oof->GetStyleDisplay()->mDisplay == NS_STYLE_DISPLAY_POPUP ||
+ !nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, oof))) {
It should probably now be
+ ((GetStateBits() & PLACEHOLDER_FOR_POPUP) ||
+ !nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, oof))) {
Attachment #467211 -
Flags: review?(roc)
Attachment #467211 -
Flags: review-
Attachment #467211 -
Flags: feedback?(fantasai.bugs)
Assignee | ||
Comment 11•15 years ago
|
||
I think this is the patch fantasai suggested. I'll call it r=dbaron unless anyone objects.
Attachment #467211 -
Attachment is obsolete: true
Attachment #467280 -
Flags: feedback?(fantasai.bugs)
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #9)
> In my stack it seems to be mitigated by frame poisoning, but in the valgrind
> stack it doesn't. Do you know why?
Because when I use valgrind for layout bugs I disable the pres arena by defining DEBUG_TRACEMALLOC_PRESARENA.
Reporter | ||
Updated•15 years ago
|
Assignee | ||
Comment 13•15 years ago
|
||
I landed fantasai's patch:
http://hg.mozilla.org/mozilla-central/rev/3c1b66168e7a
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Comment 14•15 years ago
|
||
Cool. Looks good to me. :)
Attachment #467280 -
Flags: feedback?(fantasai.bugs) → feedback+
Comment 15•15 years ago
|
||
I think you're saying this bug is a regression from bug 563584 and that's why the older branches are unaffected. I added the keyword in case we get crazy and want to land that bug on 1.9.2
Keywords: regression
Comment 16•15 years ago
|
||
Actually, it's really a mistake from bug 508473. But even if you want to land /that/ for 1.9.2, you don't have to worry about this bug because the relevant code is different in this part when ported to 1.9.2. (I know, because I noticed the mistake here by doing said porting. ;)
Updated•14 years ago
|
Crash Signature: [@ nsLineBox::IndexOf]
You need to log in
before you can comment on or make changes to this bug.
Description
•