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

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 466755 [details]
testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Created attachment 466756 [details]
crash report (from breakpad + minidump_stackwalk)

Comment 2

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 3

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable 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.

Comment 4

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Oh, except that we skip that check for floats, and I don't know why.

Comment 5

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 6

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 8

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 9

[Jesse Ruderman]

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

[fantasai]

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))) {

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 467280 [details] [diff] [review]
patch

I think this is the patch fantasai suggested.  I'll call it r=dbaron unless anyone objects.

Comment 12

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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

Comment 13

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

I landed fantasai's patch:
http://hg.mozilla.org/mozilla-central/rev/3c1b66168e7a

Comment 14

[fantasai]

Cool. Looks good to me. :)

Comment 15

[Daniel Veditz [:dveditz]]

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

Comment 16

[fantasai]

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. ;)