1500047 - Port bug 1486554 - fix static analysis errors on Windows

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Jorg K (CEST = GMT+2)]

Same as https://hg.mozilla.org/comm-central/rev/f4212f66b639

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1500047-explicit-CTORs.patch

This might do it, hard to tell locally.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5847d0a4691f4cb246f92e90f1ea93d4e10b88d5

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1500047-explicit-CTORs.patch (v2)

Now, this might do it ;-)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d52ce56f703722d1cf4ae36f381ce6088c5ef543

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1500047-explicit-CTORs.patch (v3)

One more. With that, the remaining errors are:

https://taskcluster-artifacts.net/SM4IaheIQQCh7JumWDd40g/0/public/logs/live_backing.log

z:/build/build/src/comm/mailnews/addrbook/src/nsWabAddressBook.cpp(57,16):  error: Usage of ASCII file functions (such as LoadLibraryA) is forbidden.
z:/build/build/src/comm/mailnews/mapi/mapihook/src/Registry.cpp(143,9):  error: Usage of ASCII file functions (such as LoadLibraryA) is forbidden.
z:/build/build/src/comm/mailnews/mapi/mapihook/src/Registry.cpp(178,9):  error: Usage of ASCII file functions (such as LoadLibraryA) is forbidden.

And mysteriously:
INFO -  z:/build/build/src/obj-thunderbird/dist/include/nsISupportsUtils.h(42,5):  error: 'AddRef' cannot be called on the return value of 'operator->'
INFO -      aExpr->AddRef();
I can't even see where that comes from.

Ben, could you please take a look at the AddRef() issue.

Mike and Masatoshi-san, what's the story about the LoadLibraryA calls. I tried |#undef LoadLibrary| to undo this |#define LoadLibrary  LoadLibraryA| from the SDK, but LoadLibrary is undeclared.

Comment 4

[Masatoshi Kimura [:emk]]

You should add `#define UNICODE` before #includes, or add `-DUNICODE` to build options, or use the "W" variantsdirectly (such as `LoadLibraryW`)  instead of the generic macros.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1500047-explicit-CTORs.patch (v4)

Using NS_ConvertUTF8toUTF16() or is there a better way?

I guess the run will still fail with
  error: 'AddRef' cannot be called on the return value of 'operator->'

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=473b214167b04d66be4060dae42da515d66f6af4

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 9018271 [details] [diff] [review]
1500047-explicit-CTORs.patch (v4)

> Using NS_ConvertUTF8toUTF16() or is there a better way?

Please do not. (Static analysis checker should really ban this pattern.) Windows narrow strings are not encoded in UTF-8. It will break all non-ASCII characters.

You should make WCHAR (UTF-16) strings from the start instead of converting from narrow strings.

For example, in nsWabAddressBook.cpp, you should chnage wabDLLPath to a WCHAR string, change RegQueryValueEx to RegQueryValueExW, ExpandEnvironmentStrings to ExpandEnvironmentStringsW, GetSystemDirectory to GetSystemDirectoryW, _tcscpy to wcscpy, _tcsncat to wcsncat, lstrlen to lstrlenW, and so on.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1500047-explicit-CTORs.patch (v5)

Masatoshi-san, thank you very much for the suggestion. I will do this in a follow-up, but right now I need to get the show back onto the road with anything that will compile. Since it's not UTF-8, I'll use NS_ConvertASCIItoUTF16().

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c8129a67e58fe03e4ab99936a8c3f0e08e71104b

I think I found the incorrect AddRef() as well.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4e6b66d1a7e3
Port bug 1486554: Make conversion CTORs explicit in Windows-only code and fix other minor issues. rs=bustage-fix

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 9018340 [details] [diff] [review]
1500047-explicit-CTORs.patch (v5)

I pushed this since there was other bustage to take care of. My try didn't start :-( - There are chances that this is all we need.

I will follow up replacing the NS_ConvertASCIItoUTF16() with proper "Windows W" processing as per comment #6.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1500047-take2.patch

Sadly some more:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=370fd5b518b9e3d522a7e6003fe468adbdd923b1

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9018356 [details] [diff] [review]
1500047-take2.patch

Looks like this is all that was missing.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fe6493684a2e
Port bug 1486554: Make conversion CTORs explicit in Windows-only code, take 2. rs=bustage-fix

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1500047-wchar-code.patch

This should do it.

Comment 14

[Masatoshi Kimura [:emk]]

Comment on attachment 9018433 [details] [diff] [review]
1500047-wchar-code.patch

Review of attachment 9018433 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/src/nsWabAddressBook.cpp
@@ +25,5 @@
>      if (mLibrary) { ++ mLibUsage ; return TRUE ; }
> +
> +    // Convert WAB_DLL_NAME to WCHAR.
> +    WCHAR wabDLLName[sizeof(WAB_DLL_NAME)];
> +    mbstowcs(&wabDLLName[0], WAB_DLL_NAME, sizeof(WAB_DLL_NAME));

I prefer `#define WAB_DLL_NAMEW L"" WAB_DLL_NAME` to runtime conversions.

Otherwise looks good.

Comment 15

[Ben Campbell]

Comment on attachment 9018356 [details] [diff] [review]
1500047-take2.patch

Review of attachment 9018356 [details] [diff] [review]:
-----------------------------------------------------------------

nice and simple - this one looks fine to me

Comment 16

[Ben Campbell]

Comment on attachment 9018340 [details] [diff] [review]
1500047-explicit-CTORs.patch (v5)

Review of attachment 9018340 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good from here.
I'd echo emk's NS_ConvertASCIItoUTF16 comment - It's pretty rare that I've ever seen a dll/exe with a non-ascii name, but that's my english-centric background. Anyway. It's good to be pedantic about character encoding, and your last patch seems to cover that nicely!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a8bb4bbd32e6
change some Windows-only code to WCHAR. r=emk

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #14)
> I prefer `#define WAB_DLL_NAMEW L"" WAB_DLL_NAME` to runtime conversions.
Thanks, I didn't know that his was possible.