Bug 1442275

Stop using PR_LoadLibrary on Windows

RESOLVED FIXED in Firefox 61

Get help with this page

Status

()

Product:

Component:

Type:

enhancement

Status:

RESOLVED FIXED

Reported:

a year ago

Modified:

a year ago

People

(Reporter: emk, Assigned: emk)

Tracking

Version:

unspecified

Target:

mozilla61

Points:

---

Blocks:

1440886

Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments)

Bug 1442275 - Stop using PR_LoadLibrary in GSSAPI. a year ago Masatoshi Kimura [:emk] 59 bytes, text/x-review-board-request	mayhemer : review+	Details
Bug 1442275 - Stop using PR_LoadLibrary in gfx/. a year ago Masatoshi Kimura [:emk] 59 bytes, text/x-review-board-request	jgilbert : review+	Details
Bug 1442275 - Implement a path charset agnostic wrapper for PR_LoadLibraryWithFlags. a year ago Masatoshi Kimura [:emk] 59 bytes, text/x-review-board-request	froydnj : review+	Details

Masatoshi Kimura [:emk]

Assignee

Description

•

a year ago

We have to use PR_LoadLibraryWithFlags + PR_LibSpec_PathnameU instead.

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/224338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224338/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/224340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224340/

Masatoshi Kimura [:emk]

Assignee

Updated

•

a year ago

Assignee: nobody → VYV03354

Status: NEW → ASSIGNED

Honza Bambas (:mayhemer)

Comment 3

•

a year 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)

Honza Bambas (:mayhemer)

Comment 4

•

a year 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+

Masatoshi Kimura [:emk]

Assignee

Comment 5

•

a year 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)

Jeff Muizelaar [:jrmuizel]

Comment 6

•

a year ago

Comment on attachment 8955164 [details]
Bug 1442275 - Stop using PR_LoadLibrary in gfx/.

Punting to jgilbert

Attachment #8955164 - Flags: review?(jmuizelaar) → review?(jgilbert)

Jeff Gilbert [:jgilbert]

Comment 7

•

a year 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)

Masatoshi Kimura [:emk]

Assignee

Comment 8

•

a year 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)

Andi-Bogdan Postelnicu [:andi]

Updated

•

a year ago

Blocks: 1440886

Masatoshi Kimura [:emk]

Assignee

Comment 9

•

a year ago

Ping. This change blocks bug 1440886. See comment #8 about the reason why this change is required.

Flags: needinfo?(jgilbert)

Jeff Gilbert [:jgilbert]

Comment 10

•

a year 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+

Masatoshi Kimura [:emk]

Assignee

Comment 11

•

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

Jeff Gilbert [:jgilbert]

Comment 12

•

a year 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)

Masatoshi Kimura [:emk]

Assignee

Comment 13

•

a year 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)

Nathan Froyd [:froydnj]

Comment 14

•

a year 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)

Review commit: https://reviewboard.mozilla.org/r/239154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/239154/

Comment hidden (mozreview-request)

Comment on attachment 8955163 [details]
Bug 1442275 - Stop using PR_LoadLibrary in GSSAPI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224338/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8955164 [details]
Bug 1442275 - Stop using PR_LoadLibrary in gfx/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224340/diff/1-2/

Masatoshi Kimura [:emk]

Assignee

Comment 18

•

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

Nathan Froyd [:froydnj]

Comment 19

•

a year 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 on attachment 8970370 [details]
Bug 1442275 - Implement a path charset agnostic wrapper for PR_LoadLibraryWithFlags.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/239154/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8955163 [details]
Bug 1442275 - Stop using PR_LoadLibrary in GSSAPI.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224338/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8955164 [details]
Bug 1442275 - Stop using PR_LoadLibrary in gfx/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224340/diff/2-3/

Masatoshi Kimura [:emk]

Assignee

Comment 23

•

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

Pulsebot

Comment 24

•

a year 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

Andrei Ciure[:andrei_ciure]

Comment 25

•

a year 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

Last Resolved: a year 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.