Last Comment Bug 368863 - Crash [@ nsBlockReflowState::AddFloat] using generated content, first-line and -moz-column-count
: Crash [@ nsBlockReflowState::AddFloat] using generated content, first-line an...
Status: VERIFIED FIXED
[sg:nse?] null-deref? may be a way ar...
: crash, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 346405 415447
Blocks: 364220
  Show dependency treegraph
 
Reported: 2007-01-31 10:58 PST by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (157 bytes, text/html)
2007-01-31 10:58 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
fix (28.07 KB, patch)
2007-02-19 16:30 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
Details | Diff | Review
updated patch (31.44 KB, patch)
2007-04-20 04:32 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Review
branch patch (30.84 KB, patch)
2007-07-02 19:55 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-01-31 10:58:15 PST
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 [:mwargers] (gone per 2016-05-31 :-( ) 2007-01-31 10:58:42 PST
Created attachment 253497 [details]
testcase
Comment 2 Boris Zbarsky [:bz] 2007-02-02 15:51:44 PST
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] 2007-02-04 15:50:45 PST
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 [:bz] 2007-02-04 16:07:34 PST
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) (Exited; email my personal email if necessary) 2007-02-18 21:12:03 PST
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) (Exited; email my personal email if necessary) 2007-02-19 16:30:27 PST
Created attachment 255732 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2007-02-19 16:31:32 PST
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] 2007-03-14 11:21:35 PDT
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) (Exited; email my personal email if necessary) 2007-03-14 16:01:01 PDT
I expect this to be portable to branches quite easily. And we do need it there.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-04-19 23:27:16 PDT
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-7 (review requests must explain patch) 2007-04-20 00:03:09 PDT
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) (Exited; email my personal email if necessary) 2007-04-20 01:24:01 PDT
(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) (Exited; email my personal email if necessary) 2007-04-20 04:32:15 PDT
Created attachment 262238 [details] [diff] [review]
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-7 (review requests must explain patch) 2007-04-20 10:22:05 PDT
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) (Exited; email my personal email if necessary) 2007-04-20 14:03:03 PDT
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) (Exited; email my personal email if necessary) 2007-04-20 17:43:51 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2007-04-23 18:49:20 PDT
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] 2007-06-14 10:57:13 PDT
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) (Exited; email my personal email if necessary) 2007-06-14 20:06:44 PDT
Comment on attachment 262238 [details] [diff] [review]
updated patch

ok
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-06-14 20:09:47 PDT
but the patch in bug 346405 fixes a kinda-regression from this patch and should be landed too.
Comment 21 Daniel Veditz [:dveditz] 2007-06-22 10:50:02 PDT
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) (Exited; email my personal email if necessary) 2007-07-02 19:55:05 PDT
Created attachment 270677 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2007-07-02 20:05:12 PDT
checked in on branches.
Comment 24 Carsten Book [:Tomcat] 2007-07-04 11:57:28 PDT
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] 2007-08-20 14:01:53 PDT
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é 2008-02-03 08:50:00 PST
Seems like it caused bug 415447 on 1.8 branch
Comment 27 Bob Clary [:bc:] 2009-04-24 10:56:35 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/f77c3633acc7

Note You need to log in before you can comment on or make changes to this bug.