170225 - core dump (floating point exception) on page with font-size: 1px [@ nsTextFrame::MeasureText]

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Ashley Cambrell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20020920
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20020920

A website I was developing started to core dump mozilla from build 2002091908
(build 20020915xx doesn't core dump).

After much work I tracked it down to 
body
	{
		font-family: Verdana, Arial, Helvetica, sans-serif;
	}
	
	.menu-bottom-sep
	{
		font-size: 1px;  /* <--- 1px core dumps */
		color: #000000;
		background-color: #FFFFFF;
	}

This doesn't core dump
body
	{
	}
	
	.menu-bottom-sep
	{
		font-size: 1px;
		color: #000000;
		background-color: #FFFFFF;
	}
nor does this
body
	{
		font-family: Verdana, Arial, Helvetica, sans-serif;
	}
	
	.menu-bottom-sep
	{
		font-size: 2px;
		color: #000000;
		background-color: #FFFFFF;
	}
The webpage given in the url is the bare minimum I could get the test case down
to.  If you take out the span it doesn't core dump.

Reproducible: Always

Steps to Reproduce:
1.Load nightly build 2002190908 (20021909xx?)
2.Go to webpage in URL


Actual Results:  
(with build 2002092021)
/usr/local/tmp/package/run-mozilla.sh: line 444:  9566 Floating point
exception(core dumped) "$prog" ${1+"$@"}



Expected Results:  
Page should show a '|' at font-size 1px

Nightlies are built without symbols?  Is it worth posting the back strack?

When build 2002190908 core dumps it says
/usr/local/tmp/mozilla-19092002/run-mozilla.sh: line 444:  9607 Floating point
exception(core dumped) "$prog" ${1+"$@"}

Comment 1

[Andrew Schultz]

Attachment: stacktrace

stacktrace, linux trunk build 20020921

Comment 2

[Andrew Schultz]

marking NEW

forgot... the problem is here:
estimatedNumChars = (maxWidth - aTextData.mX) / aTs.mAveCharWidth;

aTs.mAveCharWidth is 0

==> Layout/kmcclusk for traige

Comment 3

[Andrew Schultz]

regression between trunk builds 2002091804 and 2002091921
jkeiser: this looks like a regression from bug 50998

Comment 4

[Andrew Schultz]

backing out bug 50998 fixes the crash
==> jkeiser

Comment 5

[Rick Ju]

Attachment: add 0 check

add the old zero check again.

the default value of estimateNumChars: according to John Keiser, use
textRun.mTotalNumChars + 1 

john, is it ok?

Comment 6

[Rick Ju]

Attachment: #2 patch

Comment 7

[John Keiser (jkeiser)]

Comment on attachment 100363 [details] [diff] [review]
#2 patch

r=jkeiser if you:
- change else{ to else {
  (please look at other parts of the file when you do things like this to find
out how it's done)
- add a comment saying something like "// If mAveCharWidth is zero, we can fit
the entire line"
- put this logic in a single function called GetEstimatedNumChars() or
something (it's now too big to duplicate in the code)

Comment 8

[John Keiser (jkeiser)]

Comment on attachment 100363 [details] [diff] [review]
#2 patch

Er, that's needs-work, but r=me with the suggested changes :)

Comment 9

[Rick Ju]

john, 

i rewrote the patch  and found the code is ugly adding a new function. So i 
spent much time trying to understand what the hell the code r doing and hope to
 find better method. 

1. in the first time entering the big loop, the textRun.mTotalNumC = 0 ( no
segment added yet), so....

2. to add a new func to wrap the GetEstimatedNum, i have to transfer at least 4
arguments into the func. the code looks too complicated for such a simple task.
i really dont like such code. 

3. is it using a infinite var instead of aTextRun.mTotalCharNumers ?

Just FYI:

PRUint32 
nsTextFrame::GetEstimatedNumChars(TextRun&                 aTextRun, 
                                  const nsHTMLReflowState& aReflowState,
                                  TextReflowData&          aTextData, 
                                  TextStyle&               aTs)
{
  PRInt32 estimatedNumChars;

  // Estimate the number of characters that will fit. Use 105% of the available
  // width divided by the average character width
  // If mAveCharWidth is zero, we can fit the entire line
  if (aTs.mAveCharWidth != 0) {
    estimatedNumChars = (aReflowState.availableWidth - aTextData.mX) /
aTs.mAveCharWidth;
    estimatedNumChars += estimatedNumChars / 20;
  }
  else {
    estimatedNumChars = textRun.mTotalNumChars + 1; 
  }
  return estimatedNumChars;
}

OR...  use an infinite instead of textRun.mTotalNumChars? 

PRUint32 
nsTextFrame::GetEstimatedNumChars(const nsHTMLReflowState& aReflowState,
                                  TextReflowData&          aTextData, 
                                  TextStyle&               aTs)
{
  PRInt32 estimatedNumChars;

  // Estimate the number of characters that will fit. Use 105% of the available
  // width divided by the average character width
  // If mAveCharWidth is zero, we can fit the entire line
  if (aTs.mAveCharWidth != 0) {
    estimatedNumChars = (aReflowState.availableWidth - aTextData.mX) /
aTs.mAveCharWidth;
    estimatedNumChars += estimatedNumChars / 20;
  }
  else {
    estimatedNumChars = 0xfffffff; //or sth else? is there a Macro for it? 
  }
  return estimatedNumChars;
}

any way, i still dont understand nsTextFrame well, :( the code is not very clean

Comment 10

[John Keiser (jkeiser)]

Rick, I like your idea about the infinite size = 0xfffffff.  The constant you
are looking for is PRUINT32_MAX (it was recently added in bug 72100 so you may
need to update).

Here is a way around having ten thousand parameters--it makes the function's
purpose clearer as well.

PRUint32 
nsTextFrame::EstimateNumChars(PRUint32 aAvailableWidth, PRUint32 aAverageCharWidth)
{
  // Estimate the number of characters that will fit. Use 105% of the available
  // width divided by the average character width.
  // If mAveCharWidth is zero, we can fit the entire line.
  if (aAverageCharWidth == 0) {
    return PRUINT32_MAX;
  }

  PRUint32 estimatedNumChars = aAvailableWidth / aAverageCharWidth;
  return estimatedNumChars + estimatedNumChars / 20;
}


And when you call it:
estimatedNumChars = EstimateNumChars(maxWidth - aTextData.mX, aTs.mAveCharWidth);

Comment 11

[Rick Ju]

Attachment: #3

john,
the code looks ok now. it's great. 

And the patch has passed jst's review tool ( on ur homepage :). so i guess u
can't find any style/format mistake this time.	:)

Comment 12

[Andrew Schultz]

*** Bug 170589 has been marked as a duplicate of this bug. ***

Comment 13

[Andrew Schultz]

*** Bug 170556 has been marked as a duplicate of this bug. ***

Comment 14

[John Keiser (jkeiser)]

Comment on attachment 100492 [details] [diff] [review]
#3

r=jkeiser
Excellent.  That last close brace should not get un-indented, but I will change
that at checkin.

Comment 15

[Brian Ryner (not reading)]

Comment on attachment 100492 [details] [diff] [review]
#3

sr=bryner

Comment 16

[John Keiser (jkeiser)]

Will check in upon getting home.  Rick, file a bug on getting CVS access, I will
vouch :)

Comment 17

[John Keiser (jkeiser)]

Fix checked in.

Comment 18

[Andrew Schultz]

*** Bug 171154 has been marked as a duplicate of this bug. ***

Comment 19

[Andrew Schultz]

adding stack signature