Closed Bug 1445601 Opened 2 years ago Closed 2 years ago

Stop using LoadLibrary(Ex)A

Categories

(Firefox Build System :: General, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Do you think you can help with this? It would be really nice to have 1440886 landed.
Flags: needinfo?(VYV03354)
/js/src/vtune/ is a third-party folder. Could you whitelist this folder?
Flags: needinfo?(VYV03354) → needinfo?(bpostelnicu)
Depends on: 1448382
(In reply to Masatoshi Kimura [:emk] from comment #2)
> /js/src/vtune/ is a third-party folder. Could you whitelist this folder?

I've opened 1448382 for this. It should land in m-c shortly.
Flags: needinfo?(bpostelnicu)
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Andi: if you write "bug 1448382" (with "bug" in front of the number) then Bugzilla will auto-link to that bug. It's nice!
Comment on attachment 8962124 [details]
Bug 1445601 - Stop using LoadLibraryA in replace_malloc.

glandium is a better reviewer for this one.
Attachment #8962124 - Flags: review?(n.nethercote) → review?(mh+mozilla)
Comment on attachment 8962125 [details]
Bug 1445601 - Stop using LoadLibraryA in GMP.

https://reviewboard.mozilla.org/r/230976/#review236838

::: commit-message-3beac:1
(Diff revision 1)
> +Bug 1445601 - Stop using LoadLibraryA in GMP. r=cpearce

Please explain in the commit message *why* this change needs to be made. By itself, there's no information directly in the bug (which you didn't file, admitedly) nor in any other patch here, as to why you need to make this change.

::: dom/media/gmp/GMPChild.cpp:293
(Diff revision 1)
> +
> +template <size_t N>
> +constexpr bool IsASCII(const char *const (&aList)[N])
> +{
> +  for (size_t i = 0; i < N; ++i) {
> +    if (!IsASCII(aList[i])) {

We have several implementations of this function in our tree already, I think you're better off using one of the existing implementation than rolling your own.

::: dom/media/gmp/GMPChild.cpp:294
(Diff revision 1)
> +template <size_t N>
> +constexpr bool IsASCII(const char *const (&aList)[N])
> +{
> +  for (size_t i = 0; i < N; ++i) {
> +    if (!IsASCII(aList[i])) {
> +      return false;

Your IsASCII(const char\*) variant of IsASCII scans to the end of the string checking if a non-ASCII character is in the string.

But this variant also checks each character. So for each character, you then go into the loop in IsASCII(const char\*) which scans to the end of the string. This means your algorithm is O(N^2), but it should be able to be O(N), right?

::: dom/media/gmp/GMPChild.cpp:316
(Diff revision 1)
>      "evr.dll", // MFGetStrideForBitmapInfoHeader
>      "mfplat.dll", // MFCreateSample, MFCreateAlignedMemoryBuffer, MFCreateMediaType
>      "msmpeg2vdec.dll", // H.264 decoder
>      "psapi.dll", // For GetMappedFileNameW, see bug 1383611
>    };
> +  static_assert(IsASCII(whitelist),

You should just use NS_IsAscii(const char16_t\*) or NS_IsAscii(const char\*) instead of reimplementing your own function.

::: dom/media/gmp/GMPChild.cpp:325
(Diff revision 1)
>    SplitAt(", ", aLibs, libs);
>    for (nsCString lib : libs) {
>      ToLowerCase(lib);
>      for (const char* whiteListedLib : whitelist) {
>        if (lib.EqualsASCII(whiteListedLib)) {
> -        LoadLibraryA(lib.get());
> +        LoadLibraryW(NS_ConvertASCIItoUTF16(lib).get());

Why don't you just store the whitelist strings as wchar_t or char16_t strings insted of converting them at runtime?
Attachment #8962125 - Flags: review?(cpearce) → review-
Comment on attachment 8962124 [details]
Bug 1445601 - Stop using LoadLibraryA in replace_malloc.

https://reviewboard.mozilla.org/r/230974/#review236866
Attachment #8962124 - Flags: review?(mh+mozilla) → review+
Depends on: 1449082
Depends on: 1449094
Comment on attachment 8962125 [details]
Bug 1445601 - Stop using LoadLibraryA in GMP.

https://reviewboard.mozilla.org/r/230976/#review236838

> Please explain in the commit message *why* this change needs to be made. By itself, there's no information directly in the bug (which you didn't file, admitedly) nor in any other patch here, as to why you need to make this change.

Added some comments.

> We have several implementations of this function in our tree already, I think you're better off using one of the existing implementation than rolling your own.

See below.

> Your IsASCII(const char\*) variant of IsASCII scans to the end of the string checking if a non-ASCII character is in the string.
> 
> But this variant also checks each character. So for each character, you then go into the loop in IsASCII(const char\*) which scans to the end of the string. This means your algorithm is O(N^2), but it should be able to be O(N), right?

This function takes a list of string (an array of char*), not a single string. I changed this overload to mozilla::AllOf that bug 1449094 will add.

> You should just use NS_IsAscii(const char16_t\*) or NS_IsAscii(const char\*) instead of reimplementing your own function.

Filed bug 1449082 to make it possible.

> Why don't you just store the whitelist strings as wchar_t or char16_t strings insted of converting them at runtime?

Changed the whitelist to char16_t strings.
Comment on attachment 8962125 [details]
Bug 1445601 - Stop using LoadLibraryA in GMP.

https://reviewboard.mozilla.org/r/230976/#review237348

Thanks, that looks good!
Attachment #8962125 - Flags: review?(cpearce) → review+
I have seen this patch, and it looks good, but it is going to conflict greatly with a huge refactoring of nsWindowsDllInterceptor that I am about to land.

I'd prefer to just incorporate your changes into my refactoring patches to make things easier. Does that work for you?
Flags: needinfo?(VYV03354)
(In reply to Aaron Klotz [:aklotz] from comment #16)
> I have seen this patch, and it looks good, but it is going to conflict
> greatly with a huge refactoring of nsWindowsDllInterceptor that I am about
> to land.
> 
> I'd prefer to just incorporate your changes into my refactoring patches to
> make things easier. Does that work for you?

Yes, please do!
Flags: needinfo?(VYV03354)
Comment on attachment 8962123 [details]
Bug 1445601 - Stop using LoadLibraryA in nsWindowsDllInterceptor.

https://reviewboard.mozilla.org/r/230972/#review241522

I'm marking r- on this because I made these changes to my refactoring patch. :-)
Attachment #8962123 - Flags: review?(aklotz) → review-
(In reply to Aaron Klotz [:aklotz] from comment #18)
> Comment on attachment 8962123 [details]
> Bug 1445601 - Stop using LoadLibraryA in nsWindowsDllInterceptor.
> 
> https://reviewboard.mozilla.org/r/230972/#review241522
> 
> I'm marking r- on this because I made these changes to my refactoring patch.
> :-)

What bug are we talking about?
Depends on: 1432653
Attachment #8962123 - Attachment is obsolete: true
I dropped the nsWindowsDllInterceptor part. It will be incorporated into bug 1432653.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/7a1273c1861c
Stop using LoadLibraryA in replace_malloc. r=glandium
https://hg.mozilla.org/integration/autoland/rev/4d0b4650e438
Stop using LoadLibraryA in GMP. r=cpearce
No longer depends on: 1432653
https://hg.mozilla.org/mozilla-central/rev/7a1273c1861c
https://hg.mozilla.org/mozilla-central/rev/4d0b4650e438
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.