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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: roc)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, testcase, verified1.8.0.13, verified1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.13 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse?] null-deref? may be a way around that., crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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]
(Reporter)

Comment 1

11 years ago
Created attachment 253497 [details]
testcase

Comment 2

11 years ago
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?
Flags: blocking1.9?
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
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

11 years ago
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.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.11?
Flags: blocking1.8.0.10?
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...
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #255732 - Flags: superreview?(dbaron)
Attachment #255732 - Flags: review?(dbaron)
Oh, it also fixes some weirdness in this test which causes the nominal column width to go negative because of gap width arithmetic.
Blocks: 364220
Whiteboard: [sg:nse?] null-deref, may be a way around that.
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.
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Whiteboard: [sg:nse?] null-deref, may be a way around that. → [sg:nse?] [need reviews] null-deref, may be a way around that.
I expect this to be portable to branches quite easily. And we do need it there.
Whiteboard: [sg:nse?] [need reviews] null-deref, may be a way around that. → [sg:nse?] [need r=dbaron] null-deref, may be a way around that.
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 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.
Attachment #255732 - Flags: superreview?(dbaron)
Attachment #255732 - Flags: superreview+
Attachment #255732 - Flags: review?(dbaron)
Attachment #255732 - Flags: review+
(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.
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.
Attachment #255732 - Attachment is obsolete: true
Attachment #262238 - Flags: superreview?(dbaron)
Attachment #262238 - Flags: review?(dbaron)
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?
Attachment #262238 - Flags: superreview?(dbaron)
Attachment #262238 - Flags: superreview+
Attachment #262238 - Flags: review?(dbaron)
Attachment #262238 - Flags: review+
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.
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.
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.12?

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

10 years ago
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
Status: RESOLVED → VERIFIED
Whiteboard: [sg:nse?] [need r=dbaron] null-deref, may be a way around that. → [sg:nse?] null-deref? may be a way around that.

Updated

10 years ago
Flags: blocking1.9? → in-testsuite?
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 on attachment 262238 [details] [diff] [review]
updated patch

ok
Attachment #262238 - Flags: approval1.8.1.5?
Attachment #262238 - Flags: approval1.8.0.13?
but the patch in bug 346405 fixes a kinda-regression from this patch and should be landed too.
Depends on: 346405
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
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
Attachment #262238 - Flags: approval1.8.1.5?
Attachment #262238 - Flags: approval1.8.1.5+
Attachment #262238 - Flags: approval1.8.0.13?
Attachment #262238 - Flags: approval1.8.0.13+
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.
Keywords: fixed1.8.0.13, fixed1.8.1.5
checked in on branches.
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
Keywords: fixed1.8.1.5 → verified1.8.1.5
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.
Keywords: fixed1.8.0.13 → verified1.8.0.13
Group: security

Updated

10 years ago
Depends on: 415447

Comment 26

10 years ago
Seems like it caused bug 415447 on 1.8 branch

Comment 27

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/f77c3633acc7
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsBlockReflowState::AddFloat]
You need to log in before you can comment on or make changes to this bug.