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

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
Layout
--
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Jesse Ruderman, 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)
(Assignee)

Comment 2

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

Comment 3

8 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

8 years ago
Oh, except that we skip that check for floats, and I don't know why.
(Assignee)

Updated

8 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 5

8 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

8 years ago
blocking2.0: --- → final+
(Assignee)

Comment 6

8 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

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

Comment 8

8 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

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)
(Assignee)

Comment 11

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

Comment 12

8 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

8 years ago
Group: core-security
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
(Assignee)

Updated

8 years ago
Blocks: 563584
(Assignee)

Comment 13

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