Crash [@ nsLineBox::IndexOf] with -moz-column, float

RESOLVED FIXED in mozilla2.0b5

Status

()

defect
--
critical
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jruderman, Assigned: dbaron)

Tracking

(Blocks 2 bugs, {crash, regression, testcase})

Trunk
mozilla2.0b5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:dos (critical w/out frame-poisoning)], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

No description provided.
This is rather a scary stack.  I'm still thinking about what we ought to do about it.
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.
Oh, except that we skip that check for floats, and I don't know why.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
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.
blocking2.0: --- → final+
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.
Posted patch patch (obsolete) — Splinter Review
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)
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.
In my stack it seems to be mitigated by frame poisoning, but in the valgrind stack it doesn't.  Do you know why?
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)
Posted patch patchSplinter Review
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)
(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.
Group: core-security
I landed fantasai's patch:
http://hg.mozilla.org/mozilla-central/rev/3c1b66168e7a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Cool. Looks good to me. :)
Attachment #467280 - Flags: feedback?(fantasai.bugs) → feedback+
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
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. ;)
Crash Signature: [@ nsLineBox::IndexOf]
You need to log in before you can comment on or make changes to this bug.