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.

Oops, typo, meant SHBrowseForFolderW

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?

I can only download a .ZIP build for testing.

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

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.

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.

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.

> 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.

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

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

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.

shanjian: can you review?

Comment on attachment 102608 [details] [diff] [review] implicit calls of SHFooW() Looks fine. r=shanjian

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 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");

>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"); + }

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.

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

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=kin@netscape.com

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 on attachment 102608 [details] [diff] [review] implicit calls of SHFooW() a=asa for checkin to 1.2beta (on behalf of drivers)

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.

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

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

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?

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.

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

Thanks, Torben