Clicking on "Window" menu makes mozilla crash on a 64 bit OS

VERIFIED FIXED in mozilla1.1beta

Status

P1
critical
VERIFIED FIXED
16 years ago
10 years ago

People

(Reporter: shanmu, Assigned: alecf)

Tracking

Trunk
mozilla1.1beta
DEC
OSF/1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fix in hand)

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
Mozilla crashed when I hit the Window menu in the browser. This happes with
the trunk build as well as Mozilla 1.1 beta release.

The checkin through revision 1.7  to "nsWindowDataSource.cpp"  treats 
pointers to be 32 bits, which causes trouble on a 64 bit OS.

One such example is  
nsISupportsKey key(window);   has been changed to                              
               
nsPRUint32Key key(NS_PTR_TO_INT32(window)); 

I hit the bug when closure->resultWindow   is stuffed with 32 bits
                   
closure->resultWindow = NS_STATIC_CAST(nsIXULWindow*,
NS_INT32_TO_PTR(thisKey->GetValue()));

This blocks the release of Mozilla 1.1 beta on Tru64 UNIX.
I suggest this to be changed to work on 64 bit OS.
Why was that change made?  If the desire was to avoid a strong ref, why not use
nsVoidKey instead of nsISupportsKey?

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
I think this should be fixed for 1.1.

/be
Keywords: mozilla1.1

Comment 3

16 years ago
I have no way of debugging this. Please attach a stack trace so we can determine
whats to blame heere.

Comment 4

16 years ago
I have no way of debugging this. Please attach a stack trace so we can determine
whats to blame here.
It's easy to fix if you acknowledge the bug: 64-bit pointers will be chopped to
32 bits and potentially alias one another.  I don't know that that is what is
happening, but it's a hazard that can be fixed easily, by using nsVoidKey
(void*) (which seems better than casting to an int type, anyway).

/be
Cc'ing alecf, he checked in rev 1.7.  Alec, why not use nsVoidKey?

/be
(Assignee)

Comment 7

16 years ago
I wish I knew why I decided on that, but surely nsVoidKey makes more sense :)

I'll switch it over. (I actually thought this would be assigned to me, so I'll
just take it)
Assignee: adamlock → alecf
Priority: -- → P1
Target Milestone: --- → mozilla1.1beta
(Assignee)

Comment 8

16 years ago
Created attachment 92631 [details] [diff] [review]
use nsVoidKey

oops, here's the fix.
(Assignee)

Comment 9

16 years ago
I'd really like a review here - the patch is incredibly straight forward..jag?
timeless? brendan?
Status: NEW → ASSIGNED
Component: Embedding: APIs → XP Apps: GUI Features
Whiteboard: fix in hand

Updated

16 years ago
Attachment #92631 - Flags: review+
Comment on attachment 92631 [details] [diff] [review]
use nsVoidKey

r/sr=brendan@mozilla.org -- I'm assuming nothing dangled when you changed from
strong to weak (int32-weak!) refs back in 1.7.

r/sr=brendan@mozilla.org

/be
Attachment #92631 - Flags: review+ → superreview+
(Assignee)

Comment 11

16 years ago
no it's still all weak - no ownership has changed (this isn't nsISupportsKey)
thanks for the review.
*** Bug 159245 has been marked as a duplicate of this bug. ***

Comment 13

16 years ago
Comment on attachment 92631 [details] [diff] [review]
use nsVoidKey

brendan and i collided.
r=timeless
Attachment #92631 - Flags: review+

Comment 14

16 years ago
Comment on attachment 92631 [details] [diff] [review]
use nsVoidKey

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #92631 - Flags: approval+
(Assignee)

Comment 15

16 years ago
thanks everyone, fix is in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 16

16 years ago
Over to depstein for verification
QA Contact: mdunn → depstein

Comment 17

16 years ago
verified patch against Mozilla 1.2b Mozilla/5.0 (Windows; U; WinNT4.0; en-US;
rv:1.2b) Gecko/20021026. shanmu says the crash isn't happening anymore
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.