293232 - crash when I click at the end of specified web page.

Status: RESOLVED EXPIRED

(Firefox :: General, defect)

Description

[Vitja Makarov]

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

When http://www.mcl.edu.mn/itbooks/CD2/wordlists/word02.txt is fully downloaded.
I do click at the end of page. Firefox crashes at 
#0  0x085f9a74 in nsPRUint32Key::Clone() const ()


Reproducible: Always

Steps to Reproduce:
1. visit http://www.mcl.edu.mn/itbooks/CD2/wordlists/word02.txt
2. go to the end of page
3. do a single click on a page.

Actual Results:  
signal 11

Expected Results:  
nothing

Backtrace points to #57 0x085f9ac0 in nsPRUint32Key::Clone() const ()

lots of nsPRUint32Key::Clone()

Comment 1

[Vitja Makarov]

Forget to specify firefox version: 1.0.3 from official site.

Comment 2

[Ben Fowler]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050503
Firefox/1.0+

O.K. 

Going to the URL you gave, the last few lines in my browser window were

saults
saulus
saumag
saumon
saumur
sau

So I suspect an element of incompletness somewhere. I got many messages
like this 

###!!! ASSERTION: Invalid start calculated.: 'start >=0 && start <= numLines',
file ../../../../src/layout/generic/nsBlockFrame.cpp, line 6440

Comment 3

[Ben Fowler]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050508
Firefox/1.0+

Whilst I believe that the cause of this problem is well upstream, and is 
probably due to the page having 246 828 lines and having a Y dimension of
48 130 997 - whatever we do there will likely be a practical limit to how
large a web page can be, I think that there are significant problems around 
the function GetClosestLine at 
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#6360

I have found three. In rough reverse order:

1. at line 6393, we call GetLine( ) to fill in a rect for the line we
are examining. Since we are moving down the page, the value of rect.y
should progressively increase, eventually reaching a value close to
or exactly 48130997; and this is what happens until the last call to
GetLine( ) when our rect has the values {0, 0, 0, 0}.  I haven't
checked this, but I am fairly sure that this is the code at 
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsLineBox.cpp#598
returning NS_OK with the values I report.

Regretfully, GetLine( ) returns NS_OK when things are clearly not OK: The
parameter aLineNumber is out of range. Perhaps it should return NS_YOUR_BAD
or NS_PARAMETER_ERROR.

Unfortunately, GetClosestLine( ) checks the return value, and if there was
no failure, uses the value of rect.y, which in this case is bogus, and I 
have seen this happen.

It is easy for me to change my copy of the code so that GetLine( ) does
indeed return a failure code, but it really needs someone who knows the
code to verify that this would not break anything else. (It could be argued
that any caller that relies on an error condition not being reported deserves
to be broken, but that is not my call).

2. The algorithm used by GetClosestLine( ) plays out like this. We move down
the page in steadily finer strides or steps looking for the line whose
bounding rect contains our point. In fact we halve the stride distance 
at each step, which should guarantee a short path to the wanted value with
no risk of stepping outside the range of lines.

Unfortunately, at each step we nudge the starting point one line (line
6415, and accompanying comment). Now normally this is not going to
matter, as we will find our line anyway, and the error (if any), which
is also minished by the fact that the integer arithmetic on line 6387
rounds towards zero; but in this case, to get to the very bottom of the
page (I refer you to the detail of the original report) we take fourteen
strides, and end up three, five or even seven lines to the good. This is
how we end up with a line number that is (just) out of range.

There may be a one line fix:

      // Require midpoint is appropriate before using it
      NS_ASSERTION(midpoint >=0 && midpoint <= numLines, "Invalid value of
midpoint");
      start = midpoint;
      if (numLines > 1 && start < (numLines - 1)) {
        ++start;
        shifted = numLines - start; // adjust for increment of start
      } else
        shifted = 0;

whenever we advance the start line, we make a corresponding reduction to the
stride length.

3. Although I am thoroughly in favour of comments and ASSERTIONS, this
function looks as though it dates back to a time before reviews, and to
be honest I have actually rewritten the whole of it for my tree; the
ASSERTION (which seemed to be in the wrong place) isn't needed anyway.

I have a little more work to do, but I could send a patch if you wanted
to see it, but:

1. I am not guaranteeing to fix the bug,
2. The main benefit comes from the one line change in para 2 above,
3. I think that someone needs to check my para 1 before going any further.

In short this needs time and attention from the layout team which I
doubt there is to spare for this. It might be best to future or even
WONTFIX this report, as it is tolerably certain that it only affects
over long pages, which have more than 246 827 lines, and all this code
may be thrown away in any complete rewrite as I doubt that using 32-bit 
signed integer arithmetic is set in stone for layout ...

Comment 4

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 5

[Gervase Markham [:gerv]]

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.