Port bug 1486554 - fix static analysis errors on Windows

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 64.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Posted patch 1500047-explicit-CTORs.patch (obsolete) — Splinter Review
This might do it, hard to tell locally.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5847d0a4691f4cb246f92e90f1ea93d4e10b88d5
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
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.
Attachment #9018230 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(benc)
Flags: needinfo?(VYV03354)
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.
Flags: needinfo?(VYV03354)
Flags: needinfo?(mh+mozilla)
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
Attachment #9018256 - Attachment is obsolete: true
Attachment #9018271 - Flags: feedback?(VYV03354)
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.
Attachment #9018271 - Flags: feedback?(VYV03354) → feedback-
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.
Attachment #9018271 - Attachment is obsolete: true
Flags: needinfo?(benc)
Keywords: leave-open
Version: 60 → Trunk
Pushed by mozilla@jorgk.com:
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 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.
Attachment #9018340 - Flags: review?(benc)
Comment on attachment 9018356 [details] [diff] [review]
1500047-take2.patch

Looks like this is all that was missing.
Attachment #9018356 - Flags: review?(benc)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe6493684a2e
Port bug 1486554: Make conversion CTORs explicit in Windows-only code, take 2. rs=bustage-fix
This should do it.
Attachment #9018433 - Flags: review?(VYV03354)
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.
Attachment #9018433 - Flags: review?(VYV03354) → review+
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
Attachment #9018356 - Flags: review?(benc) → review+
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!
Attachment #9018340 - Flags: review?(benc) → review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a8bb4bbd32e6
change some Windows-only code to WCHAR. r=emk
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
(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.
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.