297584 - MS IE profile migration code casts LPWSTR to |char *| and treats the result as UTF-8

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Jungshik Shin]

http://msdn.microsoft.com/library/default.asp?url=/workshop/networking/urlhist/structs/staturl.asp

|pwcsurl| of STATURL struct is LPWSTR, but it's cast to |char *| and the result
is treated as UTF-8. 

http://lxr.mozilla.org/seamonkey/source/browser/components/migration/src/nsIEProfileMigrator.cpp#557

556           // 3 - URL
557           url = statURL.pwcsUrl;
558 
559           nsDependentCString urlStr((const char *) url);

I wonder how this has ever worked....

Comment 1

[Jungshik Shin]

Attachment: patch

With this patch, I don't get any more assertion in 'UTF8 -> UTF-16 converter'
and IE history items are imported properly.
 
Btw, there may be a memory leak here because MSDN says that 'STATURL->p..url'
should be freeed by a caller, but we don't. I'll see if there's indeed a leak
in another bug.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Could this possibly be the cause of bug 235495?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Ah, reading bug 235495 comment 10 and this patch seems to indicate that is not
the case. Sorry about that, please disregard.

Comment 4

[Darin Fisher]

Comment on attachment 186182 [details] [diff] [review]
patch 

Looks good.  Do we need to worry about the memory leak?

Comment 5

[Jungshik Shin]

Attachment: update (fixes memory leaks as well)

Thanks for sr. I confirmed that there are indeed memory leaks and I just
decided to fill them up in this bug.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 186315 [details] [diff] [review]
update (fixes memory leaks as well)

ohh, fun :)

Comment 7

[Jungshik Shin]

Comment on attachment 186315 [details] [diff] [review]
update (fixes memory leaks as well)

asking for a. 
this is a low risk patch that fixes the string handling in MS IE history
importing code.

Comment 8

[Jungshik Shin]

landed on the trunk with the following line moved outside |if (|stat.pwcsUrl|)|
just in case statURL.pwcsTitle is non-null for null stat.pwcsURL.

+          if (statURL.pwcsTitle) 
+            ::CoTaskMemFree(statURL.pwcsTitle);