Closed Bug 312963 Opened 14 years ago Closed 13 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: BijuMailList, Assigned: roc)

Details

(4 keywords)

Crash Data

Attachments

(11 files)

402 bytes, text/html
Details
2.49 KB, text/html
Details
1.47 KB, text/html
Details
48.12 KB, text/html
Details
6.19 KB, text/plain
Details
20.82 KB, text/html
Details
3.24 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
12.22 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
4.51 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
2.46 KB, patch
Details | Diff | Splinter Review
16.45 KB, patch
Details | Diff | Splinter Review
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
Component: Security → Layout: Block and Inline
Product: Firefox → Core
QA Contact: firefox → layout.block-and-inline
Version: unspecified → Trunk
I see three different talkback reports, TB10836489, TB10829751, TB10825966
Assignee: nobody → roc
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #1)
> I see three different talkback reports, TB10836489, TB10829751, TB10825966

Yes, they are the one
[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.
Group: security
Keywords: crash
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.
(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.
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.
(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.
(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)

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
more Talk backs id's TB10912529 TB10829751 TB10825966 TB10824706 TB10912595
TB10912597 TB10912623 TB10915047 TB10915048 TB10915049
Ok, this is what I minimise this from the wikipedia site. The testcase crashes
for me on loading.
Keywords: testcase
Thanks Martijn for testcase,
So shall I assume javascript document.body.offsetHeight is what causing the prob
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]
OS: Windows XP → All
Hardware: PC → All
Summary: crash when * {-moz-column-width:20em; -moz-column-gap:2em;} → crash when * {-moz-column-width:20em; -moz-column-gap:2em;} [@ ReparentFrame]
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)
This is a somewhat reduced version of 204081 that crashes on trunk. It doesn't use 'position'.
204081 crashes 1.5.0.1 on OS/2 but 218762 does not
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
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.
Different core stack on Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.9a1) Gecko/20060516 Minefield/3.0a1.
This is reduced quite a bit and still crashes on trunk. I'm going to focus on this testcase for now.
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.
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.
Attachment #239466 - Flags: superreview?(dbaron)
Attachment #239466 - Flags: review?(dbaron)
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.
Attachment #239467 - Flags: superreview?(dbaron)
Attachment #239467 - Flags: review?(dbaron)
Attachment #239467 - Flags: superreview?(dbaron)
Attachment #239467 - Flags: superreview+
Attachment #239467 - Flags: review?(dbaron)
Attachment #239467 - Flags: review+
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?
Attachment #239466 - Flags: superreview?(dbaron)
Attachment #239466 - Flags: superreview+
Attachment #239466 - Flags: review?(dbaron)
Attachment #239466 - Flags: review+
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?
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.
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.)
Attachment #239601 - Flags: superreview?(dbaron)
Attachment #239601 - Flags: review?(dbaron)
(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.
(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.
BTW, all these fixes are very low-risk IMHO if we need to fix branch crashes.
Comment on attachment 239601 [details] [diff] [review]
column layout fix

Rubber-stamp r+sr=dbaron.
Attachment #239601 - Flags: superreview?(dbaron)
Attachment #239601 - Flags: superreview+
Attachment #239601 - Flags: review?(dbaron)
Attachment #239601 - Flags: review+
Checked in attachment 239601 [details] [diff] [review].
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Attachment #239466 - Flags: approval1.8.1.1?
Attachment #239466 - Flags: approval1.8.0.9?
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.
Attachment #239467 - Flags: approval1.8.1.1?
Attachment #239467 - Flags: approval1.8.0.9?
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
Attachment #239466 - Flags: approval1.8.1.1?
Attachment #239466 - Flags: approval1.8.1.1+
Attachment #239466 - Flags: approval1.8.0.9?
Attachment #239466 - Flags: approval1.8.0.9+
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
Attachment #239467 - Flags: approval1.8.1.1?
Attachment #239467 - Flags: approval1.8.1.1+
Attachment #239467 - Flags: approval1.8.0.9?
Attachment #239467 - Flags: approval1.8.0.9+
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.
Status: RESOLVED → VERIFIED
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.
Crash Signature: [@ ReparentFrame]
You need to log in before you can comment on or make changes to this bug.