368863 - Crash [@ nsBlockReflowState::AddFloat] using generated content, first-line and -moz-column-count

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See testcase, which crashes Mozilla on load.
Marking security sensitive, since it is also crashing branch builds.
In branch builds they have the [@ nsLayoutUtils::GetFloatFromPlaceholder] stack trace.

Trunk builds have this stack trace:
Talkback ID: TB28891732G
0x00000000
nsBlockReflowState::AddFloat  [mozilla\layout\generic\nsblockreflowstate.cpp, line 579]
nsLineLayout::ReflowFrame  [mozilla\layout\generic\nslinelayout.cpp, line 916]

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

The <ol> is ending up on the out-of-flow overflow list of one of the last frames for the <object>....  Shouldn't it actually end up on a non-overflow list somewhere?

Comment 3

[Daniel Veditz [:dveditz]]

Boris: what are you seeing that makes it a branch blocker? In a FF2002 dbg build it appears to be a null deref.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm seeing an unexpected frame tree that really shouldn't happen and that therefore coul lead to issues like frames being leaked, etc.  While we're being saved by the null deref, I'm not sure we want to be relying on always hitting it in all possible minor variants of this testcase...

Of course if we can show that the null deref is a necessary consequence of the frame tree breakage or at least that we always clean up the frames properly, that's different.

Comment 5

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

What's happening here is that nsInlineFrame::PullOneFrame just completely fails to reparent any floats whose placeholders might have been pulled across a block boundary...

Comment 6

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

Attachment: fix

This patch adds code to nsInlineFrame (and nsFirstLineFrame) to ensure that whenever child frames get pushed or pulled, we reparent the floats associated with any placeholders that may be moving. It fixes the crash.

The patch removes the last caller of nsFrameList::PullFrame, which we no longer call since we need to do something in between identifying the frame to be pulled, and actually changing its parent. So I removed that method.

This patch could easily be landed on the branch after some trunk baking.

Comment 7

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

Oh, it also fixes some weirdness in this test which causes the nominal column width to go negative because of gap width arithmetic.

Comment 8

[Daniel Veditz [:dveditz]]

Can we get this reviewed and trunk landed? Getting this column crasher out of the way would help us not trip over it looking for potentially exploitable ones.

Do we need a different patch for the branches? Given the column crashers that do appear exploitable this would be nice to have there I think.

Comment 9

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

I expect this to be portable to branches quite easily. And we do need it there.

Comment 10

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

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

> void nsBlockFrame::CollectFloats(aTail,

>-      CollectFloats(aFrame->GetFirstChild(nsnull), aList, aTail, aFromOverflow);
>+      CollectFloats(aFrame->GetFirstChild(nsnull), aList, aTail, aFromOverflow,
>+                    aCollectSiblings);

Why shouldn't you be passing PR_TRUE to the recursive call?

>+      PRBool havePrevBlock =
>+        irs.mLineContainer ? irs.mLineContainer->GetPrevContinuation() != nsnull : PR_FALSE;

You should never need true or false inside a ?: expression.  (Yes, you just stumbled on one of the nits that annoys me the most.)  This is just (irs.mLineContainer && irs.mLineContainer->GetPrevContinuation() != nsnull).

Comment 11

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

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

>+void
>+nsInlineFrame::ReparentFloatsForInlineChild(nsIFrame* aOurLineContainer,
>+                                            nsIFrame* aFrame)

Could you assert that this is only being called if aOurLineContainer has a previous or next continuation?  I think the overflow vs. non-overflow check on the penultimate line of the function could be a performance problem if that's not the case (although it might be anyway the first time through, when reparenting the overflow of the previous block, or something like that).

I think you also need to call ReparentFloatsForInlineChild in ReflowInlineFrame in the irs.mSetParentPointer case.  (I had actually convinced myself it was ok as is, but now I've convinced myself that you need it -- although I'm still not sure I could write a testcase for needing it.)

What do all the early returns in ReparentFloatsForInlineChild mean?  Are they dangerous?  Do they really happen?

r+sr=dbaron with these comments and those from comment 10 addressed, or with an explanation of why it's right as-is.

Comment 12

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

(In reply to comment #10)
> Why shouldn't you be passing PR_TRUE to the recursive call?

You're absolutely right, I should.

> You should never need true or false inside a ?: expression.  (Yes, you just
> stumbled on one of the nits that annoys me the most.)  This is just
> (irs.mLineContainer && irs.mLineContainer->GetPrevContinuation() != nsnull).

Good point

(In reply to comment #11)
> (From update of attachment 255732 [details] [diff] [review])
> >+void
> >+nsInlineFrame::ReparentFloatsForInlineChild(nsIFrame* aOurLineContainer,
> >+                                            nsIFrame* aFrame)
> 
> Could you assert that this is only being called if aOurLineContainer has a
> previous or next continuation?

Sure.

> I think the overflow vs. non-overflow check on
> the penultimate line of the function could be a performance problem if that's
> not the case (although it might be anyway the first time through, when
> reparenting the overflow of the previous block, or something like that).

I should really pass in isOverflowFrame as a parameter to avoid problems there. I'll revise the patch to do that.

> I think you also need to call ReparentFloatsForInlineChild in
> ReflowInlineFrame
> in the irs.mSetParentPointer case.  (I had actually convinced myself it was ok
> as is, but now I've convinced myself that you need it -- although I'm still
> not sure I could write a testcase for needing it.)

It's probably a good idea in any case.

> What do all the early returns in ReparentFloatsForInlineChild mean?  Are they
> dangerous?  Do they really happen?

+  if (!ancestor || ancestor == aOurLineContainer)
+    return;

!ancestor can't really happen. ancestor == aOurLineContainer can happen, that just means no reparenting is required.

+  nsBlockFrame* ourBlock;
+  if (NS_FAILED(aOurLineContainer->QueryInterface(kBlockFrameCID, (void**)&ourBlock)))
+    return;

This QI could fail if the "line container" was the MathML thing that contains inlines. But that doesn't break vertically so I guess we can't fail here, the ancestor == aOurLineContainer check would always succeed in this case.

+  nsBlockFrame* frameBlock;
+  if (NS_FAILED(ancestor->QueryInterface(kBlockFrameCID, (void**)&frameBlock)))
+    return;

This can't fail as long as nsBlockFrame* is the only thing returning true from IsFloatContainingBlock.

I guess I should make all early returns except ancestor == aOurLineContainer into assertion failures.

Comment 13

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

Attachment: updated patch

Update to comments. To try to avoid the ContainsFrame check becoming a problem, I've added an option to ReparentFloatsForInlineChild to process all siblings of aFrame as well; we can usually reuse the ContainsFrame check and other ancestry info for all the siblings.

There is still one case where the ContainsFrame check could be a problem --- when we're doing lazy reparenting and reflowing an inline continuation inside a block continuation. We'll have to keep checking whether the frame we pulled from inside the previous block was in the previous block's overflow list. We could optimize this by caching information about frame ancestry, but that would be complicated and I'd rather avoid it unless we really need it.

Comment 14

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

Comment on attachment 262238 [details] [diff] [review]
updated patch

r+sr=dbaron

Are you sure you don't need runtime checks for some of those odd cases you converted from returns to assertions?

Comment 15

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

Hmm. I'll convert the "no block ancestor" one from an assert back to a return. The rest should stay as just assertions I think.

Comment 16

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

checked into trunk.

I think this needs trunk bake time so I don't think we should take this on branch for the next update.

Comment 17

[Martijn Wargers (dead)]

Verified fixed, the testcase doesn't crash anymore, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070422 Minefield/3.0a4pre

Comment 18

[Daniel Veditz [:dveditz]]

roc: are you ready to land this one in 1.8.1.5, or does it need more trunk bake time? More probably doesn't help at this point.

Comment 19

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

Comment on attachment 262238 [details] [diff] [review]
updated patch

ok

Comment 20

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

but the patch in bug 346405 fixes a kinda-regression from this patch and should be landed too.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 262238 [details] [diff] [review]
updated patch

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Comment 22

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

Attachment: branch patch

I had to change some occurrences of GetNext/PrevContinuation to GetNext/PrevInFlow. I also had to add GetLineContainer to nsLineLayout.h.

Comment 23

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

checked in on branches.

Comment 24

[Carsten Book [:Tomcat]]

verified fixed 1.8.1.5 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/20070704 BonEcho/2.0.0.5pre ID:2007070403 and the testcase from comment #1 - no crash - adding verified keyword

Comment 25

[juan becerra [:juanb]]

Verified fixed on MacIntel using Thunderbird version 1.5.0.13 (20070809). Enabled JavaScript, saved testcase on my local web server, then I set the start page to the poin to the testcase url. Tbird 15012 crashes, but 15013 does not.

Comment 26

[François Gagné]

Seems like it caused bug 415447 on 1.8 branch

Comment 27

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/f77c3633acc7