ToNewUnicode calls NS_StringCloneData which allocated a new buffer, but I don't see where that gets freed in this code. The buffer should land in something that frees it when it falls out of scope. http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/nsEdgeReadingListExtractor.cpp#53

Jim, what do you think about this? Note that char16ptr_t is of type "const char16_t*" so I had to remove the const-ness of it before passing it to free(). This could also have been written as free((char16_t*)mChar16Ptr);

I'd suggest a couple changes here: 1) return failure up top on a invalid length for aDBPath. 2) get rid of ToNewUnicode and just use aDBPath.BeginReading() in place of dbPath.

Tracking for 43. Does this affect older versions as well?

First off, thanks for tidying up after me (to both of you), and sorry so much fell through the cracks. :-\ (In reply to Jim Mathies [:jimm] from comment #2) > 2) get rid of ToNewUnicode and just use aDBPath.BeginReading() in place of > dbPath. Ugh. I looked for a way to get the char pointer and got confused by the "Getting a new char * buffer from a String" header in the string guide, which was meant to only deal with wanting to change ownership, which isn't necessary here. I've tweaked that section to first, explicitly, reference the iterator one (and modified the section header to no longer include the word "new")

Comment on attachment 8657464 [details] [diff] [review] Patch v2 Review of attachment 8657464 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/migration/nsEdgeReadingListExtractor.cpp @@ +31,5 @@ > { > nsresult rv = NS_OK; > *aItems = nullptr; > > + if (aDBPath.Length() == 0) { nit - Use !aDBPath.Length()

(In reply to :Gijs Kruitbosch from comment #5) > First off, thanks for tidying up after me (to both of you), and sorry so > much fell through the cracks. :-\ np. > (In reply to Jim Mathies [:jimm] from comment #2) > > 2) get rid of ToNewUnicode and just use aDBPath.BeginReading() in place of > > dbPath. > > Ugh. I looked for a way to get the char pointer and got confused by the > "Getting a new char * buffer from a String" header in the string guide, > which was meant to only deal with wanting to change ownership, which isn't > necessary here. I've tweaked that section to first, explicitly, reference > the iterator one (and modified the section header to no longer include the > word "new") You should never feel the need to apologize for getting confused about our string classes. ;)

url: https://hg.mozilla.org/integration/fx-team/rev/b14d437728aaf5f232830d8e10de80caa62b22e0 changeset: b14d437728aaf5f232830d8e10de80caa62b22e0 user: Jared Wein <jwein@mozilla.com> date: Tue Sep 08 08:52:32 2015 -0400 description: Bug 1200656 - Reading list import from Edge leaks. r=jimm

I suspect this may have broken building with vs2015 again... 2:53.62 e:/mozilla/src/browser/components/migration/nsEdgeReadingListExtractor.cpp(59): error C2664: 'JET_ERR JetGetDatabaseFileInfoW(JET_PCWSTR,void *,unsigned long,unsigned long)': cannot convert argument 1 from 'const nsAString::char_type *' to 'JET_PCWSTR' 2:53.63 e:/mozilla/src/browser/components/migration/nsEdgeReadingListExtractor.cpp(59): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast 2:53.63 e:/mozilla/src/browser/components/migration/nsEdgeReadingListExtractor.cpp(82): error C2664: 'JET_ERR JetAttachDatabaseW(JET_SESID,JET_PCWSTR,JET_GRBIT)': cannot convert argument 2 from 'const nsAString::char_type *' to 'JET_PCWSTR' 2:53.64 e:/mozilla/src/browser/components/migration/nsEdgeReadingListExtractor.cpp(82): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast 2:53.65 e:/mozilla/src/browser/components/migration/nsEdgeReadingListExtractor.cpp(85): error C2664: 'JET_ERR JetOpenDatabaseW(JET_SESID,JET_PCWSTR,JET_PCWSTR,JET_DBID *,JET_GRBIT)': cannot convert argument 2 from 'const nsAString::char_type *' to 'JET_PCWSTR' 2:53.67 e:/mozilla/src/browser/components/migration/nsEdgeReadingListExtractor.cpp(85): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Confirmed by local backout

Marco, does this fix it for you?

Comment on attachment 8658355 [details] [diff] [review] Follow-up, use char16ptr_t Review of attachment 8658355 [details] [diff] [review]: ----------------------------------------------------------------- yes, this builds.