Closed Bug 1442275 Opened 7 years ago Closed 7 years ago

Stop using PR_LoadLibrary on Windows

Categories

(Core :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(3 files)

We have to use PR_LoadLibraryWithFlags + PR_LibSpec_PathnameU instead.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
(In reply to Masatoshi Kimura [:emk] from comment #0) > We have to use PR_LoadLibraryWithFlags + PR_LibSpec_PathnameU instead. Can you please refer the motivation here?
Flags: needinfo?(VYV03354)
Attachment #8955163 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #3) > (In reply to Masatoshi Kimura [:emk] from comment #0) > > We have to use PR_LoadLibraryWithFlags + PR_LibSpec_PathnameU instead. > > Can you please refer the motivation here? PR_LoadLibrary uses 8-bit strings on Windows that are lossy on Windows.
Flags: needinfo?(VYV03354)
Comment on attachment 8955164 [details] Bug 1442275 - Stop using PR_LoadLibrary in gfx/. Punting to jgilbert
Attachment #8955164 - Flags: review?(jmuizelaar) → review?(jgilbert)
(In reply to Masatoshi Kimura [:emk] from comment #5) > (In reply to Honza Bambas (:mayhemer) from comment #3) > > (In reply to Masatoshi Kimura [:emk] from comment #0) > > > We have to use PR_LoadLibraryWithFlags + PR_LibSpec_PathnameU instead. > > > > Can you please refer the motivation here? > > PR_LoadLibrary uses 8-bit strings on Windows that are lossy on Windows. Why do you want to load non-ASCII paths? We prefer to keep things ASCII where possible.
Flags: needinfo?(VYV03354)
(In reply to Jeff Gilbert [:jgilbert] from comment #7) > Why do you want to load non-ASCII paths? We prefer to keep things ASCII > where possible. We have no control over the path characterss. If user name contains non-ASCII characters, both per-user installed installation paths and profile paths will contain non-ASCII characters as well. So we have to deal with non-ASCII characters for virtually all file operations.
Flags: needinfo?(VYV03354)
Blocks: 1440886
Ping. This change blocks bug 1440886. See comment #8 about the reason why this change is required.
Flags: needinfo?(jgilbert)
Comment on attachment 8955164 [details] Bug 1442275 - Stop using PR_LoadLibrary in gfx/. https://reviewboard.mozilla.org/r/224340/#review236842 ::: gfx/gl/GLContextProviderWGL.cpp:96 (Diff revision 1) > > if (!mOGLLibrary) { > - mOGLLibrary = PR_LoadLibrary(libGLFilename.c_str()); > + PRLibSpec libSpec; > + libSpec.type = PR_LibSpec_PathnameU; > + libSpec.value.pathname_u = libGLFilename.c_str(); > + mOGLLibrary = PR_LoadLibraryWithFlags(libSpec, 0); This pattern should really be pulled out into a util function, since we're using it everywhere now. ::: gfx/vr/gfxVROSVR.cpp:121 (Diff revision 1) > static PRLibrary* osvrClientLib = nullptr; > static PRLibrary* osvrClientKitLib = nullptr; > //this looks up the path in the about:config setting, from greprefs.js or modules\libpref\init\all.js > //we need all the libs to be valid > - nsAutoCString osvrUtilPath, osvrCommonPath, osvrClientPath, osvrClientKitPath; > - if (NS_FAILED(mozilla::Preferences::GetCString("gfx.vr.osvr.utilLibPath", > +#ifdef XP_WIN > + constexpr static auto* GetPathStringPref = mozilla::Preferences::GetString; s/GetPathStringPref/pfnGetPathStringPref/ Function pointer variable names should not look like real function names. ::: gfx/vr/gfxVROSVR.cpp:125 (Diff revision 1) > - nsAutoCString osvrUtilPath, osvrCommonPath, osvrClientPath, osvrClientKitPath; > - if (NS_FAILED(mozilla::Preferences::GetCString("gfx.vr.osvr.utilLibPath", > - osvrUtilPath)) || > - NS_FAILED(mozilla::Preferences::GetCString("gfx.vr.osvr.commonLibPath", > - osvrCommonPath)) || > - NS_FAILED(mozilla::Preferences::GetCString("gfx.vr.osvr.clientLibPath", > +#ifdef XP_WIN > + constexpr static auto* GetPathStringPref = mozilla::Preferences::GetString; > +#else > + constexpr static auto* GetPathStringPref = mozilla::Preferences::GetCString; > +#endif > + nsTAutoString<mozilla::filesystem::Path::value_type> This explicit templatization seems fragile and obtuse. Either typedef something in the ifdef, or move the declaration into the ifdef. ::: gfx/vr/gfxVROculus.cpp:640 (Diff revision 1) > -#else > -#error "Unsupported platform!" > -#endif > > // search the path/module dir > - libSearchPaths.InsertElementsAt(0, 1, nsCString()); > + libSearchPaths.InsertElementsAt(0, 1, EmptyString()); Why not nsString()?
Attachment #8955164 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #10) > ::: gfx/gl/GLContextProviderWGL.cpp:96 > (Diff revision 1) > > > > if (!mOGLLibrary) { > > - mOGLLibrary = PR_LoadLibrary(libGLFilename.c_str()); > > + PRLibSpec libSpec; > > + libSpec.type = PR_LibSpec_PathnameU; > > + libSpec.value.pathname_u = libGLFilename.c_str(); > > + mOGLLibrary = PR_LoadLibraryWithFlags(libSpec, 0); > > This pattern should really be pulled out into a util function, since we're > using it everywhere now. We have nsIFile::Load. Can I use XPCOM under gfx/gl/ or gfx/vr/? If not, I'll implement a corresponding function under MFBT.
(In reply to Masatoshi Kimura [:emk] from comment #11) > (In reply to Jeff Gilbert [:jgilbert] from comment #10) > > ::: gfx/gl/GLContextProviderWGL.cpp:96 > > (Diff revision 1) > > > > > > if (!mOGLLibrary) { > > > - mOGLLibrary = PR_LoadLibrary(libGLFilename.c_str()); > > > + PRLibSpec libSpec; > > > + libSpec.type = PR_LibSpec_PathnameU; > > > + libSpec.value.pathname_u = libGLFilename.c_str(); > > > + mOGLLibrary = PR_LoadLibraryWithFlags(libSpec, 0); > > > > This pattern should really be pulled out into a util function, since we're > > using it everywhere now. > > We have nsIFile::Load. Can I use XPCOM under gfx/gl/ or gfx/vr/? If not, > I'll implement a corresponding function under MFBT. Please include it in MFBT.
Flags: needinfo?(jgilbert)
Hmm, but can I depend on NSPR headers in MFBT? If not, where is the best place to put the PR_LoadLibraryWithFlags wrapper?
Flags: needinfo?(nfroyd)
(In reply to Masatoshi Kimura [:emk] from comment #13) > Hmm, but can I depend on NSPR headers in MFBT? If not, where is the best > place to put the PR_LoadLibraryWithFlags wrapper? I think you can depend on NSPR headers in MFBT so long as those bits are guarded by #ifdef MOZILLA_INTERNAL_API, since SpiderMonkey can use MFBT headers, but (I think?) doesn't have to use NSPR.
Flags: needinfo?(nfroyd)
Comment on attachment 8955164 [details] Bug 1442275 - Stop using PR_LoadLibrary in gfx/. https://reviewboard.mozilla.org/r/224340/#review236842 > Why not nsString()? I heard Empty(C)String() should be used for empty strings.
Comment on attachment 8970370 [details] Bug 1442275 - Implement a path charset agnostic wrapper for PR_LoadLibraryWithFlags. https://reviewboard.mozilla.org/r/239154/#review246976 ::: mfbt/SharedLibrary.h:19 (Diff revision 1) > +inline PRLibrary* > +#ifdef XP_WIN > +LoadLibraryWithFlags(char16ptr_t aPath, PRUint32 aFlags) > +#else > +LoadLibraryWithFlags(const char* aPath, PRUint32 aFlags) Documentation for this would be good...at least for what `aFlags` is. Maybe `PRUint32 aFlags = 0`, and modify all callsites to not pass 0?
Attachment #8970370 - Flags: review?(nfroyd) → review+
Comment on attachment 8970370 [details] Bug 1442275 - Implement a path charset agnostic wrapper for PR_LoadLibraryWithFlags. https://reviewboard.mozilla.org/r/239154/#review246976 > Documentation for this would be good...at least for what `aFlags` is. > > Maybe `PRUint32 aFlags = 0`, and modify all callsites to not pass 0? Added a comment and a default parameter.
Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/af62a7677b23 Implement a path charset agnostic wrapper for PR_LoadLibraryWithFlags. r=froydnj https://hg.mozilla.org/integration/autoland/rev/26a288cf0e0b Stop using PR_LoadLibrary in GSSAPI. r=mayhemer https://hg.mozilla.org/integration/autoland/rev/54978536db3e Stop using PR_LoadLibrary in gfx/. r=jgilbert
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: