170969 - Mozilla doesn't start on Windows 95

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

Last known working build ID: 2002092304
Known non-working build ID: 2002092506

This bug was probably exposed by bug 104934.

SHBrowserForFolderW and SHGetPathFromIDListW don't exist on Windows 95.

Comment 1

[[e-mail]]

Oops, typo, meant SHBrowseForFolderW

Comment 2

[Roy Yokoyama]

wow.  Win95 platform!! It's 7 yrs old OS.  
neil: I think I can make a patch to fix it; but none of us have Win95 for
      dev/testing.  Would you be able to test my patch once available?

Comment 3

[[e-mail]]

I can only download a .ZIP build for testing.

Comment 4

[Roy Yokoyama]

neil: ok.  I found a qa Win95 machine.  I believe we can test the patch here. :)
      thanks. 

Comment 5

[(not reading, please use [e-mail] instead)]

do we still support win95?  I don't even think that MSFT does.

the only reason I ask is there will probably be more changes down the road that 
aren't going to work for win95.

Comment 6

[timeless]

this one is easily fixable. unicode on w95 is supported by microsoft unicode for
windows, so i can't think of anything which would require us to break w95 compat.

Comment 7

[Roy Yokoyama]

Attachment: Create nsToolkit helper functions

Comment 8

[Asa Dotzler [:asa]]

We support what our development community is willing to maintain. If there are
no developers willing to keep us working on win95 then we die on win95. 
That being said, we work pretty well on win95 and I don't think we should let
any easily fixed bugs kill that platform. 

Comment 9

[[e-mail]]

> wow.  Win95 platform!! It's 7 yrs old OS

> do we still support win95?

Well, Unicode support on 9x is patchy - sometimes (as in this case) the function
doesn't exist (perhaps I need to install IE but that defeats the object :-) ),
while sometimes the function exists but doesn't work. Mozilla even almost works
on Windows NT 3.51 - the only unsupported call Mozilla uses is
SystemParametersInfo( SPI_GETNONCLIENTMETRICS, ... )

I really appreciate your taking the time to fix this.

Comment 10

[[e-mail]]

It it me or has "Attachments" > "Save All" stopped working in Windows 98?

Comment 11

[Torben]

FWIW build 2002093004 works on win95 since the patch to bug 104934 was backed out.

Comment 12

[Roy Yokoyama]

Attachment: implicit calls of SHFooW()

Need to add 
nsToolkit::mSHGetPathFromIDList = &nsSHGetPathFromIDList; 
nsToolkit::mSHBrowseForFolder = &nsSHBrowseForFolde;
to the 104934 attachment(already /r and /sr)

One unique thing about this is that we need to get pointer of
SHFooW() by implicitly calling GetProcAdress()
(NS_SHGetPathFromIDList)GetProcAddress(module, "SHGetPathFromIDListW"); 
(NS_SHBrowseForFolder)GetProcAddress(module, "SHBrowseForFolderW"); 

need review.

Comment 13

[Roy Yokoyama]

shanjian: can you review?

Comment 14

[Shanjian Li]

Comment on attachment 102608 [details] [diff] [review]
implicit calls of SHFooW()

Looks fine. r=shanjian

Comment 15

[Roy Yokoyama]

kin: can you super review?  

This is yet another nsToolkit:: prefix stuff.
As much as you dislike the ifdef's,
http://bugzilla.mozilla.org/show_bug.cgi?id=171228#c9 , 
I would have to ask you for /sr.
#ifdef's will eventually be removed from the tree, soon. :) 

Comment 16

[kinmoz]

Comment on attachment 102608 [details] [diff] [review]
implicit calls of SHFooW()

==== Can this ever return null or fail? If so, what's our fallback so we don't
call through a null pointer in nsFilePicker.cpp?


+	 nsToolkit::mSHGetPathFromIDList =
(NS_SHGetPathFromIDList)GetProcAddress(module, "SHGetPathFromIDListW"); 
+	 nsToolkit::mSHBrowseForFolder =
(NS_SHBrowseForFolder)GetProcAddress(module, "SHBrowseForFolderW");

Comment 17

[Roy Yokoyama]

>Can this ever return null or fail?
I thought of the same. It should never be null in NT base system. 
I have wrapped the call with (nsToolkit::mIsNT) && (module) 
just to be safe. If these conditions fail, then they fall 
back to A version.

I am not sure about non-Microsoft supported Windows
simulation though (eg. WINE in Linux, etc); but who would want
to run moz for Windows in Linux when we have the native
mozilla for Linux......

I can add ASSERTION as follow:
+  if (!nsToolkit::mDefWindowProc || 
+    !nsToolkit::mCallWindowProc ||
+    !nsToolkit::mSetWindowLong ||
+    !nsToolkit::mSendMessage ||
+    !nsToolkit::mDispatchMessage ||
+    !nsToolkit::mGetMessage ||
+    !nsToolkit::mPeekMessage ||
+    !nsToolkit::mGetOpenFileName ||
+    !nsToolkit::mGetSaveFileName ||
+    !nsToolkit::mGetClassName ||
+    !nsToolkit::mCreateWindowEx ||
+    !nsToolkit::mRegisterClass ||
+    !nsToolkit::mSHGetPathFromIDList ||
+    !nsToolkit::mSHBrowseForFolder) {
+      NS_ASSERTION(0, "Wide API doesn't exist");
+  }    

Comment 18

[[e-mail]]

Surely
+  NS_ASSERTION(
+    nsToolkit::mDefWindowProc &&
+    nsToolkit::mCallWindowProc &&
+    nsToolkit::mSetWindowLong &&
+    nsToolkit::mSendMessage &&
+    nsToolkit::mDispatchMessage &&
+    nsToolkit::mGetMessage &&
+    nsToolkit::mPeekMessage &&
+    nsToolkit::mGetOpenFileName &&
+    nsToolkit::mGetSaveFileName &&
+    nsToolkit::mGetClassName &&
+    nsToolkit::mCreateWindowEx &&
+    nsToolkit::mRegisterClass &&
+    nsToolkit::mSHGetPathFromIDList &&
+    nsToolkit::mSHBrowseForFolder,
+    "Wide API doesn't exist");
This saves code in release builds.

Comment 19

[Roy Yokoyama]

I'll take neil's suggestion. :)
kin: good for /sr?

Comment 20

[kinmoz]

Comment on attachment 102608 [details] [diff] [review]
implicit calls of SHFooW()

==== I don't think it's necessary to check all the non mSH* pointers with your
assertion given the fact that the *W symbols have to be defined before the app
will even start up/link. It's the dynamic result of GetProcAddress() that
worries me and the fact that we assign the values directly into the mSH*
pointers, so we can't even fallback on the nsSH* versions we have. Would it be
better to assign into temporaries first? Or do something like this so we can
fallback on the nsSH* versions they are originally set to?

+	 nsToolkit::mSHGetPathFromIDList =
(NS_SHGetPathFromIDList)GetProcAddress(module, "SHGetPathFromIDListW"); 
+	 if (!nsToolkit::mSHGetPathFromIDList)
+	   nsToolkit::mSHGetPathFromIDList = &nsSHGetPathFromIDList;
+	   
+	 nsToolkit::mSHBrowseForFolder =
(NS_SHBrowseForFolder)GetProcAddress(module, "SHBrowseForFolderW");
+	 if (!nsToolkit::mSHBrowseForFolder)
+	   nsToolkit::mSHBrowseForFolder = &nsSHBrowseForFolder;

==== By the way, you might already know this, but I don't think it's necessary
to use '&' in front of the function names when getting their pointers.


sr=[e-mail]

Comment 21

[Roy Yokoyama]

I'll add 
+	 nsToolkit::mSHGetPathFromIDList =
(NS_SHGetPathFromIDList)GetProcAddress(module, "SHGetPathFromIDListW"); 
+	 if (!nsToolkit::mSHGetPathFromIDList)
+	   nsToolkit::mSHGetPathFromIDList = &nsSHGetPathFromIDList;
+	   
+	 nsToolkit::mSHBrowseForFolder =
(NS_SHBrowseForFolder)GetProcAddress(module, "SHBrowseForFolderW");
+	 if (!nsToolkit::mSHBrowseForFolder)
+	   nsToolkit::mSHBrowseForFolder = &nsSHBrowseForFolder;

and remove & in front of the function names.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 102608 [details] [diff] [review]
implicit calls of SHFooW()

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

Comment 23

[Roy Yokoyama]

fix checked into the trunk
Please verify once we turn the MOZ_UNICODE flag on by default
or provide a special unicode build (whichever comes first)
Making 104934 as a depend.

Comment 24

[Rui Xu]

Tested with trunk 101815-unicode build on EN, JA and SC Win95OSR2, works fine.

Comment 25

[Torben]

Latest nightly (build 2002102510 probably, cannot check since it does not start)
shows exactly same behaviour as the builds hit with this bug.

Comment 26

[Roy Yokoyama]

Torben: We haven't turn the Unicode flag for the regular trunk build just yet.  
        Our iQAs were testing against the special Unicode build 
        which is available from 
       
http://ftp.mozilla.org/pub/mozilla/nightly/2002-10-23-08-unicode/mozilla-win32.zip 
        Which build are you testing with?

Comment 27

[Torben]

Re #26:
This was a trunk build, probably 2002102508-trunk judging by the modification
time. If unicode is not turned on yet I guess it is another bug (sigh...). I
will test more on monday and fill a new bug if necessary.

Comment 28

[Torben]

Filled bug 177083 to adress the new crashes noted in comment 25. Unicode builds
2002102308 and 2002102408 works fine on win 95.

Comment 29

[Roy Yokoyama]

Thanks, Torben