Mozilla crashed on a Tru64 UNIX machine

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
Layout
P2
critical
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Shanmugavelu Shanmuganathan (gone), Assigned: karnaze (gone))

Tracking

({crash})

Trunk
mozilla1.0.1
DEC
OSF/1
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
Mozilla 1.0 crahsed while trying to access www.tamilbeat.com
I loaded this page and clicked on the second item on the right.
Mozilla crashed.
It didn't crash if when I clicked the first item (or any oter item).
Upon debugging I found that the following. 
In ./layout/html/table/src/nsTableFrame.cpp
the line 5918 reads like this
 for (PRInt32 rowX = info.rowIndex; rowX <= cellEndRowIndex; rowX++) {
nsTableRowFrame* rowFrame = (rowX == info.rowIndex) ? info.topRow
:rowFrame->GetNextRow();
....
 }
Here rowFrame's scope is inside the for loop. Tru64 UNIX uses
the memory that is freed and there is no gaurantee that the address
will still hold the same value after a while.  So once mozilla steps out
of this loop the memory will be used. So when the next time this 
value is accessed it get the garbage value and crashes. To avoid this 
it would be safe to declare rowFrame outside of the loop. 

  nsTableRowFrame* rowFrame;
      for (PRInt32 rowX = info.rowIndex; rowX <= cellEndRowIndex; rowX++) {
        rowFrame = (rowX == info.rowIndex) ? info.topRow : rowFrame->GetNextRow();
        ....
      }
I made these changes and it worked fine and didn't crash. 

I found the same exact code in another place in the same  file.
I will attach the fix soon.
(Reporter)

Comment 1

16 years ago
Created attachment 86501 [details] [diff] [review]
Fix
-> NEW

Reporter: Please ask for reviews for your patch (see www.mozilla.org/hacking)
Severity: major → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, review
Summary: Mozilla crashed on a Tru64 UNIX machine → Mozilla crashed on a Tru64 UNIX machine
(Reporter)

Comment 3

16 years ago
Can I have a r= and sr= for this patch please.

Updated

16 years ago
QA Contact: petersen → moied
-> Karnaze
Assignee: attinasi → karnaze

Updated

16 years ago
Priority: -- → P2
(Assignee)

Comment 5

16 years ago
Comment on attachment 86501 [details] [diff] [review]
Fix

r=karnaze
Attachment #86501 - Flags: review+

Comment 6

16 years ago
Comment on attachment 86501 [details] [diff] [review]
Fix

sr=kin@netscape.com
Attachment #86501 - Flags: superreview+
(Reporter)

Comment 7

16 years ago
I don't have permissions to commit my changes in to the repository.
may I ask some one in this list to commit these changes.
(Assignee)

Comment 8

16 years ago
The patch is in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 9

16 years ago
This commit have added two "may be used uninitialized" warnings:

+layout/html/table/src/nsTableFrame.cpp:5917
+ `class nsTableRowFrame * rowFrame' might be used uninitialized in this function
+
+layout/html/table/src/nsTableFrame.cpp:5950
+ `class nsTableRowFrame * rowFrame' might be used uninitialized in this function

See also bug 59652 and bug 59675
(Assignee)

Comment 10

16 years ago
shanmu? - this is your patch
(Reporter)

Comment 11

16 years ago
These two warnings always showed up. I just had a glimpse of the build log
from one of the clobber build (before the fix was checked in), where I see this.

Warning 156: "nsTableFrame.cpp", line 5918 # The variable 'rowFrame' may not be
initialized on all possible paths from start of the function.
    bleRowFrame* rowFrame = (rowX == info.rowIndex) ? info.topRow :
rowFrame->GetNextRow();
                                                                    ^^^^^^^^   
           
Warning 156: "nsTableFrame.cpp", line 5950 # The variable 'rowFrame' may not be
initialized on all possible paths from start of the function.
    bleRowFrame* rowFrame = (rowX == info.rowIndex) ? info.topRow :
rowFrame->GetNextRow();


                                                                   

Comment 12

16 years ago
Yes, this has always shown up.  If you look at the logic...
yes it is "ok" because the first time thru the loop
you don't access rowframe, only on subsequent times thru
the loop.

I had suggested a change to remove these warnings because
in this loop this check is unnecessary (and wasteful).
But never checked it in...

Comment 13

16 years ago
*** Bug 136769 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Whiteboard: [FIXED_ON_TRUNK]
Target Milestone: --- → mozilla1.0.1
(Assignee)

Updated

16 years ago
Keywords: adt1.0.1, mozilla1.0.1, nsbeta1

Updated

16 years ago
Keywords: nsbeta1 → nsbeta1+

Comment 14

16 years ago
Comment on attachment 86501 [details] [diff] [review]
Fix

a=chofmann for the 1.01 branch. add fixed1.01 after checking in.  thanks
Attachment #86501 - Flags: approval+

Updated

16 years ago
Keywords: mozilla1.0.1 → mozilla1.0.1+

Comment 15

16 years ago
added adt1.0.1+
Keywords: adt1.0.1 → adt1.0.1+
(Assignee)

Comment 16

16 years ago
fixed1.0.1
Keywords: fixed1.0.1
Whiteboard: [FIXED_ON_TRUNK]

Updated

16 years ago
Keywords: mozilla1.0.1+
*** Bug 155661 has been marked as a duplicate of this bug. ***
*** Bug 159010 has been marked as a duplicate of this bug. ***

Comment 19

16 years ago
Verified with branch build 20020731 on winxp, adding KW:verified1.0.1
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1
You need to log in before you can comment on or make changes to this bug.