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

RESOLVED FIXED in mozilla2.0b5

Status

()

--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jruderman, Assigned: dbaron)

Tracking

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

Trunk
mozilla2.0b5
crash, regression, testcase
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)

(Reporter)

Description

8 years ago
Created attachment 466755 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Comment 1

8 years ago
Created attachment 466756 [details]
crash report (from breakpad + minidump_stackwalk)
Created attachment 467165 [details]
valgrind's explanation of the problem

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.
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.
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.
Created attachment 467211 [details] [diff] [review]
patch

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.
(Reporter)

Comment 9

8 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

8 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)
Created attachment 467280 [details] [diff] [review]
patch

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.
(Reporter)

Updated

8 years ago
Group: core-security
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
I landed fantasai's patch:
http://hg.mozilla.org/mozilla-central/rev/3c1b66168e7a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5

Comment 14

8 years ago
Cool. Looks good to me. :)

Updated

8 years ago
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

Comment 16

8 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. ;)
Crash Signature: [@ nsLineBox::IndexOf]
You need to log in before you can comment on or make changes to this bug.