312963 - crash when * {-moz-column-width:20em; -moz-column-gap:2em;} [@ ReparentFrame]

Status: VERIFIED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Biju]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051018 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051018 Firefox/1.6a1

I can consistently crash Firefox while surfing a maliciously crafted website.
Tested in 2 Win XP PC at different network, 
one running SP2 and other not running SP2.

Steps to reproduce crash
pre-condition : latest Firefox/1.6a1

Easy way 
1. open userContent.css to modify
2. add rule "* { -moz-column-width:20em; -moz-column-gap:2em; }"
3. restart Firefox
4. go to http://en.wikipedia.org/ 
5. continue surfing pages by click links
6. After few pages (<10) Firefox will crash

I was also able to crash at http://yahoo.com in few more (>15) pages surfing.

Alternative way, ie with CSS on a web site

1. create a login at http://en.wikipedia.org/ 
2. create user css file"monobook.css" (all lowercase)
eg:- http://en.wikipedia.org/wiki/User:BijuGC/monobook.css 
3. close http://en.wikipedia.org/ 
4. clear cache
5. go to http://en.wikipedia.org/ 
6. continue surfing pages by click links
7. After few pages (<10) Firefox will crash


Reproducible: Always

Steps to Reproduce:
see above
Actual Results:  
Firefox/1.6a1 crashed

Expected Results:  
Firefox/1.6a1 should not crash

For Talkback crash ID, i forgot collect/do not know how get it.
So please check for -moz-column-width and -moz-column-gap in Talkback comments

Comment 1

[Daniel Veditz [:dveditz]]

I see three different talkback reports, TB10836489, TB10829751, TB10825966

Comment 2

[Biju]

(In reply to comment #1)
> I see three different talkback reports, TB10836489, TB10829751, TB10825966

Yes, they are the one

Comment 3

[Daniel Veditz [:dveditz]]

[process note: You can get the talkback id's by running the talkback executable
found under your installation directory. If you include something useful in the
comments (as you did) you can search for them at
http://talkback-public.mozilla.org/. If you include your e-mail address then
some of the talkback insiders can search based on that, but that's not publicly
available for privacy reasons.]

ugh, looks terrible. I'm mostly not getting 20em columns, often usually they're
only one word wide. On the 1.8 branch there's horrible column overlap, but I
don't crash. On the trunk the gap appears wider than the columns, and I do
eventually crash as you describe.

I mostly crashed in ReparentFrame() (like your TB10836489) calling GetParent on
a null aFrame. Crashed once in nsBlockFrame::DrainOverflowLines (like
TB10829751), also trying to call GetParent on a null frame (fpif).

Got lots of assertions before the crashes about "bad geometric parent
'mFrames.ContainsFrame(aNextInFlow)'" at nsContainerFrame.cpp line 1023

null dereferences are not an exploitable security issue, clearing flag.

Comment 4

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

There are known problems with abs-pos children, e.g. bug 288357. Without
specific testcases it's hard to know what's going on here.

Comment 5

[Biju]

(In reply to comment #3)
> [process note: 

thanks for info, now I see talkback exe, i did know they changed the dir

> ugh, looks terrible. I'm mostly not getting 20em columns, 
> often usually they're only one word wide. 

that may be because most web sites leave a margin on left to show menu.

I was realy exited about the column-width feature, I wished this feature to be
there in HTML since 1996.

Comment 6

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

You can use columns in your own web site, you just need to be careful about
absolute positioning and avoid letting the columns overflow. It's just not
possible to take arbitrary pages and column-ize them like you're trying to do.
See http://weblogs.mozillazine.org/roc for an example which works pretty well IMHO.

Comment 7

[Biju]

(In reply to comment #6)
> It's just not
> possible to take arbitrary pages and column-ize them
> like you're trying to do.

Sorry, that was not my intension.

The above test is only for proving firefox can be crashed from a malformed website.

Comment 8

[Martijn Wargers (dead)]

(In reply to comment #7)
> Sorry, that was not my intension.
No need to apologize, it is good of you that you filed this crash bug. The bug
needs a minimal testcase, though.
Is there a particular page that crashes with these css rules? Or does it just
happen 'randomly' after a while (which would make it hard to make a testcase)

Comment 9

[Biju]

It may or may not be random, as once you set * {-moz-column-width:20em;
-moz-column-gap:2em;} it is hard to see any page properly. 

So after setting I go to http://en.wikipedia.org/wiki/Main_Page 

and continue clicking what ever appear to me (a wild guess) as an article link.
once new page comes click another link at that page
continue doing it for few time
after few time (< 10) Firefox will crash

I was able reproduce more faster at http://en.wikipedia.org/ 
than yahoo.com of cnn.com

Comment 10

[Biju]

more Talk backs id's TB10912529 TB10829751 TB10825966 TB10824706 TB10912595
TB10912597 TB10912623 TB10915047 TB10915048 TB10915049

Comment 11

[Martijn Wargers (dead)]

Attachment: testcase (crashes on load)

Ok, this is what I minimise this from the wikipedia site. The testcase crashes
for me on loading.

Comment 12

[Biju]

Thanks Martijn for testcase,
So shall I assume javascript document.body.offsetHeight is what causing the prob

Comment 13

[Adam Guthrie]

Incident ID: 10915049
Stack Signature	ReparentFrame a8eed732
Product ID	FirefoxTrunk
Build ID	2005101906
Trigger Time	2005-10-20 19:22:01.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	firefox.exe + (00237ed2)
URL visited	http://www.mozilla.org/projects/deerpark/
User Comments	https://bugzilla.mozilla.org/show_bug.cgi?id=312963 { -moz-column-width:20em; -moz-column-gap:2em; }
Since Last Crash	4 sec
Total Uptime	344 sec
Trigger Reason	Access violation
Source File, Line No.	c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 560
Stack Trace 	
ReparentFrame  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 560]
nsBlockFrame::PullFrameFrom  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2886]
nsBlockFrame::PullFrame  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2819]
nsBlockFrame::DoReflowInlineFrames  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 3909]
nsBlockFrame::ReflowInlineFrames  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 3747]
nsBlockFrame::ReflowLine  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2742]
nsBlockFrame::ReflowDirtyLines  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2276]
nsBlockFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 910]
nsContainerFrame::ReflowChild  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 891]
nsColumnSetFrame::ReflowChildren  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsColumnSetFrame.cpp, line 481]
nsColumnSetFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsColumnSetFrame.cpp, line 743]
nsBlockReflowContext::ReflowBlock  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockReflowContext.cpp, line 606]
nsBlockFrame::ReflowBlockFrame  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 3461]
nsBlockFrame::ReflowLine  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2623]
nsBlockFrame::ReflowDirtyLines  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2276]
nsBlockFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 910]
nsContainerFrame::ReflowChild  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 891]
nsColumnSetFrame::ReflowChildren  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsColumnSetFrame.cpp, line 481]
nsColumnSetFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsColumnSetFrame.cpp, line 743]
nsBlockReflowContext::ReflowBlock  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockReflowContext.cpp, line 606]
nsBlockFrame::ReflowBlockFrame  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 3461]
nsBlockFrame::ReflowLine  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2623]
nsBlockFrame::ReflowDirtyLines  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2276]
nsBlockFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 910]
nsContainerFrame::ReflowChild  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 891]
nsColumnSetFrame::ReflowChildren  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsColumnSetFrame.cpp, line 481]
nsColumnSetFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsColumnSetFrame.cpp, line 853]
nsBlockReflowContext::ReflowBlock  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockReflowContext.cpp, line 606]
nsBlockFrame::ReflowBlockFrame  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 3461]
nsBlockFrame::ReflowLine  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2623]
nsBlockFrame::ReflowDirtyLines  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 2276]
nsBlockFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 910]
nsContainerFrame::ReflowChild  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 891]
CanvasFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsHTMLFrame.cpp, line 534]
nsContainerFrame::ReflowChild  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 891]
nsHTMLScrollFrame::ReflowScrolledFrame  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsGfxScrollFrame.cpp, line 530]
nsHTMLScrollFrame::ReflowContents  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsGfxScrollFrame.cpp, line 593]
nsHTMLScrollFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsGfxScrollFrame.cpp, line 793]
nsContainerFrame::ReflowChild  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 891]
ViewportFrame::Reflow  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsViewportFrame.cpp, line 239]
IncrementalReflow::Dispatch  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 859]
PresShell::ProcessReflowCommands  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6492]
PresShell::WillPaint  [c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6136]

Comment 14

[Felix Miata]

Attachment: another testcase (crashes on load)

Just load this in 1.5rc3 or trunk - instant crash.
Private email said talkback ID TB12135855E gives same backtrace as this bug
I tried further reduction without success.
Several ways to change the testcase to not crash:
1-comment away -moz-column-count and -moz-column-gap in the styles
2-remove the commented div
3-insert any or all of the missing close quotes in the 4 spans (doing all 4 makes the HTML validate)

Comment 15

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

Attachment: somewhat reduced version of attachment 204081

This is a somewhat reduced version of 204081 that crashes on trunk. It doesn't use 'position'.

Comment 16

[Felix Miata]

204081 crashes 1.5.0.1 on OS/2 but 218762 does not

Comment 17

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; de-AT; rv:1.8.0.2) Gecko/20060404 SeaMonkey/1.0.1

crashed by loading Attachment 204081 [details], newer Attachment 218762 [details] does not crash

Comment 18

[Hermann Schwab]

Attachment: TB17680806G crash with Attachment 204081 Firefox 1.5.0.2

Mozilla/5.0 (Windows; U; Win98; de; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
crash using Attachment 204081 [details]: another testcase (crashes on load)

As the older TBs are deleted, here TB17680806G saved as html with working links to bonsai.

Comment 19

[Alfred Peng]

Attachment: Core file by attachment 200332

Different core stack on Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.9a1) Gecko/20060516 Minefield/3.0a1.

Comment 20

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

Attachment: reduced further from attachment 218762

This is reduced quite a bit and still crashes on trunk. I'm going to focus on this testcase for now.

Comment 21

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

Okay, I have a swag of fixes that fix all the crashes and layout issues in these testcases. I'll break them up into separate patches.

Comment 22

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

Attachment: don't add frames to the same line as continuation placeholders

nsBlockFrame::AddFrames can add frames to the same line as a line of continuation placeholders. This is not allowed; we have an invariant that lines containing continuation placeholders contain nothing else. This violation is triggered by attachment #200332 [details].

The patch is a simple extension of the existing logic to decide whether to put a frame on the same line in AddFrames.

Comment 23

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

Attachment: Honour aTryPull when pulling inline frames

ReflowDirtyLines takes an aTryPull parameter which controls whether it should pull frames from the next-in-flow. We set this to false when we're reflowing after choosing our shrink-wrap width, because we don't want to change where we broke vertically. Unfortunately we only honoured aTryPull when we were pulling *lines* from the next-in-flow block, and did not honour it when we pulled inline frames one by one from the next-in-flow block. This patch fixes that oversight. Pretty simple.

Comment 24

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

Comment on attachment 239466 [details] [diff] [review]
don't add frames to the same line as continuation placeholders

r+sr=dbaron

Are there any assertions that yell about this invariant?  Could there be, easily?

Comment 25

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

Comment on attachment 239467 [details] [diff] [review]
Honour aTryPull when pulling inline frames

r+sr=dbaron

Could the caller(s) that sets aTryPull to false easily assert that nothing is pulled?

Comment 26

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

Before I forget, here's the gist of the next upcoming patch:

Column reflow assumes that if we reflow the content with height Y and the max of the column frame heights is then Y' < Y, that we could reflow again with height Y' and everything would fit.

This can be violated. For example, the content might contain a block with a fixed height, but which overflows vertically. When reducing the available height from Y to Y', that block might break vertically, and its continuation might not fit in the available number of columns. This happens in attachment #218762 [details].

The upcoming patch fixes the two sites where we use that assumption. One is when we're doing incremental reflow and we skip reflowing a column because the available height is decreasing and the column's height is less than the new available height. Here we just check the overflow rect's ymost instead. (There can be overflowing content and yet the column still fits in the available height, but in that case we just give up on the optimization.)

For the other site, the reflow balancing algorithm, I haven't quite decided the best strategy yet. I have two that work :-). I'll finish that off tomorrow.

Comment 27

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

Attachment: column layout fix

Fixes the broken assumption I mentioned in the previous comment. I fixed balancing by just using the overflow-rect to compute maxHeight, similar to the way I fixed incremental reflow. I also added code to note that when the last reflow allowed all content to fit, both maxHeight and mLastBalanceHeight are feasible. (It used to be that maxHeight <= mLastBalanceHeight so we only needed to check maxHeight. now that's no longer true because a column can overflow vertically and yet fit (e.g. with relative positioning), so we have to check them separately.)

Comment 28

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

(In reply to comment #24)
> (From update of attachment 239466 [details] [diff] [review] [edit])
> r+sr=dbaron
> 
> Are there any assertions that yell about this invariant?  Could there be,
> easily?

Yes, there's one in DrainOverflowLines, which helped me find the bug.

I checked this in when I checked in the fix for bug 350137.

Comment 29

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

(In reply to comment #25)
> (From update of attachment 239467 [details] [diff] [review] [edit])
> r+sr=dbaron
> 
> Could the caller(s) that sets aTryPull to false easily assert that nothing is
> pulled?

Not easily, I think.

Comment 30

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

Checked in attachment #239467 [details] [diff] [review].

Comment 31

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

BTW, all these fixes are very low-risk IMHO if we need to fix branch crashes.

Comment 32

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

Comment on attachment 239601 [details] [diff] [review]
column layout fix

Rubber-stamp r+sr=dbaron.

Comment 33

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

Checked in attachment 239601 [details] [diff] [review].

Comment 34

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

Comment on attachment 239466 [details] [diff] [review]
don't add frames to the same line as continuation placeholders

Layout crash fix. Been on trunk for ages, no regression reports. Low risk.

Comment 35

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

Comment on attachment 239467 [details] [diff] [review]
Honour aTryPull when pulling inline frames

Layout crash fix. Been on trunk for ages, no regression reports. Low risk.

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 239466 [details] [diff] [review]
don't add frames to the same line as continuation placeholders

approved for 1.8/1.8.0 branch, a=dveditz for drivers

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 239467 [details] [diff] [review]
Honour aTryPull when pulling inline frames

approved for 1.8/1.8.0 branch, a=dveditz for drivers

Comment 38

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

Attachment: Branch version of attachment #239466

This required some merging.

Comment 39

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

Attachment: branch version of attachment #239467

This also required some merging.

Comment 40

[Martijn Wargers (dead)]

Verified fixed using the latest trunk build. I don't crash with either of the 3 testcases attached.
On the latest branch builds, though, I still crash, I filed bug 362291 for that.

Comment 41

[Jay Patel [:jay]]

v.fixed on 1.8.0 and 1.8.1 branches with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.9) Gecko/20061206 Firefox/1.5.0.9
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

No crash with the somewhat reduced testcase (attachment 218762 [details]).  We can deal with the remaining crash in the new bug 362291.