400244 - Crash [@ nsSplittableFrame::GetNextInFlow] with -moz-column, -moz-appearance

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase makes Firefox crash [@ nsSplittableFrame::GetNextInFlow] dereferencing 0xddddde0d.

The testcase uses -moz-appearance, so it's probably Mac-specific.  I wasn't able to eliminate the use of -moz-appearance with the usual trick of replacing it with "width" and "height" properties.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: simplified testcase

This testcase is a simplified version of the first testcase. It doesn't crash, but it does trigger "WARNING: nsBlockFrame::CheckFloats: Explicit float list is out of sync with float cache", which will probably lead us to the root cause of the crash.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixing that warning is pretty easy, but it doesn't fix the crash.

What's happening here is quite complex. <sigh>.

Basically we get into a situation where the first column contains the <br>, the second column contains the the margin part of the float, and the third column contains the rest of the float.

Then we reflow the first column. After placing the <br> line, it pulls the second column's line back to the first column in case it fits. This pulls the float's second placeholder out of the third column and saves it in the first column's nsBlockReflowState::mOverflowPlaceholders so we can eventually destroy it or put it in the first column's overflow list as needed. We reflow the pulled line: before reflowing the line, DoReflowInlineFrames saves the current mOverflowPlaceholders.LastChild() into lastPlaceholder. When we reflow the line, the float's first-in-flow decides a) it will map all the content and be complete but b) it doesn't fit where it is and should be pushed to the next block continuation. Because of a), it destroys the second placeholder, removing it from mOverflowPlaceholders. lastPlaceholder is now a dangling pointer. Because of b), DoReflowInlineFrames then decides to push the entire line to the overflow-list. It wants to destroy overflow placeholders created by this reflow of the line; so it calls PushTruncatedPlaceholderLine passing lastPlaceholder as the frame we should "back up" mOverflowPlaceholders to; i.e. we call UndoSplitplaceholders to destroy all placedholders after lastPlaceholder in the mOverflowPlaceholders list.

But because lastPlaceholder is a dangling pointer, we crash.

I need to think about how to fix this. I wonder if we can just get rid of UndoSplitPlaceholders completely. It seems to me it's safe to leave those unnecessary continuations alive and in mOverflowPlacholders. They'll get pushed to the end of the block's overflow-list and eventually destroyed when the float(s) get completely reflowed.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Removing UndoSplitPlaceholders completely fixes this bug, and as far as I can tell, it's safe to do. Reftests pass, for what that's worth.

This patch also contains a change to ReflowInlineFrame to treat frames that are both NS_FRAME_IS_TRUNCATED and NS_FRAME_NOT_COMPLETE as NS_FRAME_IS_TRUNCATED (i.e. push the line to the next column/page) instead of taking the NS_FRAME_NOT_COMPLETE path. This fixes the CheckFloats warning.

Not sure who to ask for review here. Requesting from dbaron because he probably should be in the loop, and from fantasai because I think she's thought about this part of the code.

Comment 5

[fantasai]

-    // XXX we probably should be doing something with oof here. But what?

So.. this was leaving oof frames lying around without a placeholder? (Just checking if I understand what was going on.)

+ // if the frame is a placeholder and was complete but truncated

I believe you need to delete "and was complete".

Does the float's first-in-flow get reflowed after it gets pushed to the next line?

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> So.. this was leaving oof frames lying around without a placeholder? (Just
> checking if I understand what was going on.)

Maybe, but I didn't notice that specific problem. In particular that is not what was causing the warning and crash here.

> I believe you need to delete "and was complete".

OK.

> Does the float's first-in-flow get reflowed after it gets pushed to the next
> line?

Usually it will be reflowed when we reflow the next column/page, but that doesn't always happen. For example when balancing two columns, we might have content in the overflow list for the second column which does not get reflowed before we change heights and reflow everything again starting with the first column. This shouldn't cause a problem. Everything should eventually be removed from overflow lists one way or another, but we try not to rely on that...

Comment 7

[fantasai]

Ok, let me rephrase that: if reflow is not interrupted, does the float's first-in-flow get reflowed after it gets pushed to the next line before we finish (the reflow root's) reflow?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, normally we'll reflow the line when we reflow the current block's next in flow.

Comment 9

[fantasai]

Comment on attachment 286911 [details] [diff] [review]
fix

r=fantasai with that comment fix

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Marking P2 because the fix should land for beta2 as there might be regressions. (And this has security implications so we can't not fix it.)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 286911 [details] [diff] [review]
fix

sr=dbaron (mostly rubber-stamp); sorry for the delay.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 13

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007123104 Minefield/3.0b3pre and the testcase from this bug - no crash on testcase

-> Verified fixed

Comment 14

[Jesse Ruderman]

The testcase doesn't trigger any assertions/crashes on branch.

Comment 15

[Jesse Ruderman]

Crashtest checked in.