Closed
Bug 1442275
Opened 7 years ago
Closed 7 years ago
Stop using PR_LoadLibrary on Windows
Categories
(Core :: General, enhancement)
Core
General
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
![]() |
||
Comment 3•7 years ago
|
||
(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)
![]() |
||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8955163 [details]
Bug 1442275 - Stop using PR_LoadLibrary in GSSAPI.
https://reviewboard.mozilla.org/r/224338/#review231330
Attachment #8955163 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 5•7 years ago
|
||
(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 6•7 years ago
|
||
Comment on attachment 8955164 [details]
Bug 1442275 - Stop using PR_LoadLibrary in gfx/.
Punting to jgilbert
Attachment #8955164 -
Flags: review?(jmuizelaar) → review?(jgilbert)
Comment 7•7 years ago
|
||
(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)
Assignee | ||
Comment 8•7 years ago
|
||
(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)
Assignee | ||
Comment 9•7 years ago
|
||
Ping. This change blocks bug 1440886. See comment #8 about the reason why this change is required.
Flags: needinfo?(jgilbert)
Comment 10•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 11•7 years ago
|
||
(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.
Comment 12•7 years ago
|
||
(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)
Assignee | ||
Comment 13•7 years ago
|
||
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)
![]() |
||
Comment 14•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
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 19•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review-reply |
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.
Comment 24•7 years ago
|
||
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
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af62a7677b23
https://hg.mozilla.org/mozilla-central/rev/26a288cf0e0b
https://hg.mozilla.org/mozilla-central/rev/54978536db3e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•