943291 - widget/windows/nsDataObj.cpp should use Unicode APIs

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Henri Sivonen (:hsivonen)]

widget/windows/nsDataObj.cpp calls nsPrimitiveHelpers::ConvertUnicodeToPlatformPlainText. Instead, it should use Unicode system APIs to make such conversion unnecessary.

Comment 1

[Masatoshi Kimura [:emk]]

We have to convert to system-locale-dependent string for compatibility. But we could use WideCharToMultiByte(CP_ACP, ...) because this code is Windows-specific anyway.

Comment 2

[u518207]

It seems that we need to allocate buffer for plainTextData. 
What is the allocation type, or in other words what is the right method to use for an allocation. 
I couldn't find what is the original allocation type.

Comment 3

[Masatoshi Kimura [:emk]]

Hm, things are really screwed up.
nsSaveAsCharset::Convert is using PR_Malloc() to allocate the data which are supposed to be freed by PR_Free():
https://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/nsSaveAsCharset.cpp?rev=b81d755b1b37#169
One consumer is using free() to free the allocated buffer:
https://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsDragService.cpp?rev=6c1c7e45c902#1423
Another consumer is using nsMemory:Free():
https://mxr.mozilla.org/mozilla-central/source/widget/windows/nsDataObj.cpp?rev=1a64b22632bc#1277

Comment 4

[u518207]

Attachment: 943291.patch

The second if statement uses nsMemory::Alloc() for allocation and the respective  nsMemory:Free() like you mentioned. To conform with it my choice was nsMemory::Alloc() for memory allocation. Moreover, PR_Malloc() was unrecognized and it gave me some motivation to look for the alternative.
The gtk's free() method wasn't modified because I can't check it; my compilation process seems to ignore gtk. I compile in Windows with VS.

Comment 5

[u518207]

Attachment: 943291.patch

Oh, missed the null termination.

Comment 6

[Masatoshi Kimura [:emk]]

Comment on attachment 8493248 [details] [diff] [review]
943291.patch

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

::: widget/windows/nsDataObj.cpp
@@ +1270,5 @@
>    DWORD allocLen = (DWORD)len;
>    if ( aFE.cfFormat == CF_TEXT ) {
>      // Someone is asking for text/plain; convert the unicode (assuming it's present)
>      // to text with the correct platform encoding.
> +    

extra space

@@ +1272,5 @@
>      // Someone is asking for text/plain; convert the unicode (assuming it's present)
>      // to text with the correct platform encoding.
> +    
> +    // 512 reserved char positions.
> +    size_t bufferSize = sizeof(char)*(len / 2 + 1 + 512);

Where 512 come from? The buffer will be insufficient if len > 512.
One Unicode character will be converted to at most two multibyte character. So |len + 2| will be sufficient.

@@ +1278,4 @@
>      char16_t* castedUnicode = reinterpret_cast<char16_t*>(data);
>      int32_t plainTextLen = 0;
> +    plainTextLen = WideCharToMultiByte(CP_ACP, 0, (LPCWSTR)castedUnicode, len / 2 + 1, plainTextData, bufferSize, NULL, NULL);
> +    plainTextLen = plainTextLen ? plainTextLen - 1 : 0;

If WideCharToMultiByte fails, one byte garbage will be copied to the destination.

Comment 7

[u518207]

Attachment: 943291-v2.patch

The 512 were from here:
https://mxr.mozilla.org/mozilla-central/source/intl/unicharutil/nsSaveAsCharset.cpp?rev=b81d755b1b37#187
they are named RESERVE_FALLBACK_BYTES in the original allocation.
Removed those additional bits.

You are right, sometimes this function translates wide character into two characters.
Modifications are performed.

Comment 8

[Masatoshi Kimura [:emk]]

Comment on attachment 8493578 [details] [diff] [review]
943291-v2.patch

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

::: widget/windows/nsDataObj.cpp
@@ +1276,5 @@
>      char16_t* castedUnicode = reinterpret_cast<char16_t*>(data);
>      int32_t plainTextLen = 0;
> +    plainTextLen = WideCharToMultiByte(CP_ACP, 0, (LPCWSTR)castedUnicode, len / 2 + 1, plainTextData, bufferSize, NULL, NULL);
> +    // If WideCharToMultiByte fails, one byte garbage will be copied to the destination.
> +    plainTextLen = plainTextLen ? plainTextLen - 1 : 1;

Could you propagate the failure to the caller instead of copying the garbage?
See the else clause below about the error handling.

Comment 9

[u518207]

Attachment: 943291-v3.patch

Here is next modification.

Comment 10

[Masatoshi Kimura [:emk]]

Comment on attachment 8494371 [details] [diff] [review]
943291-v3.patch

Looks good to me.

Comment 11

[u518207]

I tried to run mochitest, but it returns an error
93198 ERROR Automation Error: Received unexpected exception while running application. In my opinion it is not related to this patch.
Maybe to try running another test.

Comment 12

[Masatoshi Kimura [:emk]]

I pushed your patch to try:
https://tbpl.mozilla.org/?tree=Try&rev=63b2e8d2ca2e

Comment 13

[u518207]

Thanks.

Comment 14

[Masatoshi Kimura [:emk]]

Try looks green, landing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b9c4085e11f

Thanks for the contribution to Mozilla!

Comment 15

[u518207]

That's good.
Thanks for guidance!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b9c4085e11f

Comment 17

[Henri Sivonen (:hsivonen)]

Thank you for fixing this! One step closer to getting rid of nsIPlatformCharset.

Comment 18

[u518207]

Good.