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

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[Shanmugavelu Shanmuganathan (gone)]

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.

Comment 1

[Brendan Eich [:brendan]]

Why was that change made?  If the desire was to avoid a strong ref, why not use
nsVoidKey instead of nsISupportsKey?

/be

Comment 2

[Brendan Eich [:brendan]]

I think this should be fixed for 1.1.

/be

Comment 3

[Adam Lock]

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

Comment 4

[Adam Lock]

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

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Brendan Eich [:brendan]]

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

/be

Comment 7

[Alec Flett]

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)

Comment 8

[Alec Flett]

Attachment: use nsVoidKey

oops, here's the fix.

Comment 9

[Alec Flett]

I'd really like a review here - the patch is incredibly straight forward..jag?
timeless? brendan?

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 92631 [details] [diff] [review]
use nsVoidKey

r/sr=[e-mail] -- I'm assuming nothing dangled when you changed from
strong to weak (int32-weak!) refs back in 1.7.

r/sr=[e-mail]

/be

Comment 11

[Alec Flett]

no it's still all weak - no ownership has changed (this isn't nsISupportsKey)
thanks for the review.

Comment 12

[Matthias Versen [:Matti]]

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

Comment 13

[timeless]

Comment on attachment 92631 [details] [diff] [review]
use nsVoidKey

brendan and i collided.
r=timeless

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 92631 [details] [diff] [review]
use nsVoidKey

a=asa (on behalf of drivers) for checkin to 1.1

Comment 15

[Alec Flett]

thanks everyone, fix is in.

Comment 16

[Michael Dunn]

Over to depstein for verification

Comment 17

[David Epstein]

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