Change from 'this' to L'this' and remove the cast that was hiding this mismatch.

Change from 'this' to L'this' and remove the cast that was hiding this mismatch.

Found by VC++'s /analyze. Warning was:

fpdfsdk\src\javascript\js_runtime.cpp(352) : warning C6276:
Cast between semantically different string types:  char * to wchar_t *.
Use of invalid string can lead to undefined behavior.

This mismatch has been there as far back as the history goes (to May of this year).

It looks like a real bug to me. However I don't know the implications of this bug and why it would not have been noticed at run-time.

The code has been this way as far back as the git history goes, but that is only to May 2014.

Original patch from Bruce Dawson(brucedawson@chromium.org)

BUG=427616

[brucedawson, 2014-11-08 01:52:26]

brucedawson@chromium.org changed reviewers:
+ jabdelmalek@google.com

[brucedawson, 2014-11-08 01:52:37]

Can you take a look at this?

[Bo Xu, 2014-11-14 00:05:51]

On 2014/11/08 01:52:37, brucedawson wrote:
> Can you take a look at this?

Thanks brucedawson, this is indeed a bug. LGTM.

The commit-bot does not work for Pdfium, so you have to use "git cl land" to
land it.

[cpu_(ooo_6.6-7.5), 2014-11-14 18:34:05]

Bo Xu, he is not a chromium committer so I think git cl land would not work with
his credentials.

If nobody get around to do this I'll do it next week.

[Bo Xu, 2014-11-14 18:46:16]

On 2014/11/14 18:34:05, cpu wrote:
> Bo Xu, he is not a chromium committer so I think git cl land would not work
with
> his credentials.
> 
> If nobody get around to do this I'll do it next week.

@brucedawson, in the end of the description, can you add something like
"Original patch from Bruce Dawson(brucedawson@chromium.org)", so I will land
this for you. Thanks.

[Bo Xu, 2014-11-14 21:37:43]

On 2014/11/08 01:52:37, brucedawson wrote:
> Can you take a look at this?

We might want to write as 

array.Add((FX_LPCWSTR)L"this"); 

to make it consistent with other places of using this.

[brucedawson, 2014-11-14 21:41:57]

The cast is what allowed this bug to happen in the first place so I think
getting rid of the cast is important.

Also, looking through the rest of the pdfium source code I see three other
instances of "this" and none of them have a cast.

BTW, is there any concern about what this might break? The three comparisons of
sObjName against L"this" have been failing for years and will now start passing.
Is this going to break anything? This is a behavior changing fix, which always
has to be handled carefully.

[Bo Xu, 2014-11-14 22:14:40]

On 2014/11/14 21:41:57, brucedawson wrote:
> The cast is what allowed this bug to happen in the first place so I think
> getting rid of the cast is important.
> 
> Also, looking through the rest of the pdfium source code I see three other
> instances of "this" and none of them have a cast.
> 
> BTW, is there any concern about what this might break? The three comparisons
of
> sObjName against L"this" have been failing for years and will now start
passing.
> Is this going to break anything? This is a behavior changing fix, which always
> has to be handled carefully.

Right, I see. It seems the other places of (FX_LPCWSTR)L"xxx" in pdfium is not
good as well.

[brucedawson, 2014-11-14 22:22:27]

Agreed. I don't know if any of the other casts are causing problems (I don't see
any signs of it) but they should be removed. In many cases they are completely
unneeded. In other cases it would be safer to replace a CFX_WideString being
cast to FX_LPCWSTR with a call to .Get() -- this makes it more obvious what is
happening, and it cannot be misused to hide a bug.

But, that is a separate issue. I'll file a bug to suggest that.

[Bo Xu, 2014-11-14 23:13:19]

On 2014/11/14 22:22:27, brucedawson wrote:
> Agreed. I don't know if any of the other casts are causing problems (I don't
see
> any signs of it) but they should be removed. In many cases they are completely
> unneeded. In other cases it would be safer to replace a CFX_WideString being
> cast to FX_LPCWSTR with a call to .Get() -- this makes it more obvious what is
> happening, and it cannot be misused to hide a bug.
> 
> But, that is a separate issue. I'll file a bug to suggest that.

Thanks Dawson. The patch has been committed to
https://pdfium.googlesource.com/pdfium/+/d66fda28dc2159ffc68c312764172382825c...

[Tom Sepez, 2014-11-14 23:46:20]

Drive-by: Note also the casts on or about line 333 are redundant, and should be
removed to avoid masking the same error should the code get borked in the
future:

	sRet.Replace((FX_LPCWSTR)L"_", (FX_LPCWSTR)L".");

[Bo Xu, 2014-11-14 23:51:23]

On 2014/11/14 23:46:20, Tom Sepez wrote:
> Drive-by: Note also the casts on or about line 333 are redundant, and should
be
> removed to avoid masking the same error should the code get borked in the
> future:
> 
> 	sRet.Replace((FX_LPCWSTR)L"_", (FX_LPCWSTR)L".");

Right, there are a whole bunch of these casts. We should clean them per #9.

[Tom Sepez, 2014-11-14 23:57:06]

On 2014/11/14 23:46:20, Tom Sepez wrote:
> Drive-by: Note also the casts on or about line 333 are redundant, and should
be
> removed to avoid masking the same error should the code get borked in the
> future:
> 
> 	sRet.Replace((FX_LPCWSTR)L"_", (FX_LPCWSTR)L".");

See
https://code.google.com/p/chromium/codesearch#search/&q=%5C(FX_LPCWSTR%5C)%5C...
There are a number of these; the casts aren't doing anything in that either the
literal is of the right type or it isn't and the cast doesn't fix that, it just
hides mistakes.  Can we pull all of these?

[Bo Xu, 2014-11-15 00:10:13]

On 2014/11/14 23:57:06, Tom Sepez wrote:
> On 2014/11/14 23:46:20, Tom Sepez wrote:
> > Drive-by: Note also the casts on or about line 333 are redundant, and should
> be
> > removed to avoid masking the same error should the code get borked in the
> > future:
> > 
> > 	sRet.Replace((FX_LPCWSTR)L"_", (FX_LPCWSTR)L".");
> 
> See
>
https://code.google.com/p/chromium/codesearch#search/&q=%5C(FX_LPCWSTR%5C)%5C...
> There are a number of these; the casts aren't doing anything in that either
the
> literal is of the right type or it isn't and the cast doesn't fix that, it
just
> hides mistakes.  Can we pull all of these?

I will do a clean up.

[brucedawson, 2014-11-15 00:18:28]

I can CR the clean up. Many of the casts can be removed. Those that currently
serve a purpose will need to be evaluated to see if there is a better option.
There usually is. In those cases where the cast is there to force a conversion
from CFX_WideString to FX_LPCWSTR it is better (IMHO) to use .Get() to do the
conversion explicitly and avoid the potential for hiding bugs.

[Bo Xu, 2014-11-15 00:40:47]

On 2014/11/15 00:18:28, brucedawson wrote:
> I can CR the clean up. Many of the casts can be removed. Those that currently
> serve a purpose will need to be evaluated to see if there is a better option.
> There usually is. In those cases where the cast is there to force a conversion
> from CFX_WideString to FX_LPCWSTR it is better (IMHO) to use .Get() to do the
> conversion explicitly and avoid the potential for hiding bugs.

At lease for the wchar_t* literals, we can safely remove the cast, right? I have
a patch at https://codereview.chromium.org/726143002/ for this.

[brucedawson, 2014-11-15 01:25:41]

On 2014/11/15 00:40:47, Bo Xu wrote:
> On 2014/11/15 00:18:28, brucedawson wrote:
> > I can CR the clean up. Many of the casts can be removed. Those that
currently
> > serve a purpose will need to be evaluated to see if there is a better
option.
> > There usually is. In those cases where the cast is there to force a
conversion
> > from CFX_WideString to FX_LPCWSTR it is better (IMHO) to use .Get() to do
the
> > conversion explicitly and avoid the potential for hiding bugs.
> 
> At lease for the wchar_t* literals, we can safely remove the cast, right? I
have
> a patch at https://codereview.chromium.org/726143002/ for this.

Yep, that's good progress.