Last Comment Bug 768317 - Don't try to save a reference to the internal buffer of NS_ConvertUTF16toUTF8 in DirPrefObserver
: Don't try to save a reference to the internal buffer of NS_ConvertUTF16toUTF8...
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Kent James (:rkent)
Depends on:
  Show dependency treegraph
Reported: 2012-06-25 21:11 PDT by Kent James (:rkent)
Modified: 2012-06-28 10:41 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Somebody needs to own the prefname storage (980 bytes, patch)
2012-06-26 09:25 PDT, Kent James (:rkent)
mozilla: review+
Details | Diff | Review

Description Kent James (:rkent) 2012-06-25 21:11:27 PDT
My AB unit tests in ExQuilla were acting strangely, and I traced it to this core code, given here with the disassembly:

(in nsDirPrefs.cpp):

   const char *prefname = NS_ConvertUTF16toUTF8(aData).get();
5B49B8D1  mov         eax,dword ptr [aData] 
5B49B8D4  push        eax  
5B49B8D5  lea         ecx,[ebp-6Ch] 
5B49B8D8  call        NS_ConvertUTF16toUTF8::NS_ConvertUTF16toUTF8 (597FAF02h) 
5B49B8DD  mov         ecx,eax 
5B49B8DF  call        nsCString::get (59891470h) 
5B49B8E4  mov         dword ptr [prefname],eax 
5B49B8E7  lea         ecx,[ebp-6Ch] 
5B49B8EA  call        NS_ConvertUTF16toUTF8::~NS_ConvertUTF16toUTF8 (597C5375h) 

  DIR_PrefId id = DIR_AtomizePrefName(prefname);

What I see in the debugger is that aData makes sense, but *prefname is a long string containing only "Z". Stepping through the assembler, the conversion is done correctly but gets overwritten in NS_ConvertUTF16toUTF8::~NS_ConvertUTF16toUTF8

The code is not correct. You are trying to hold onto the reference to the internal buffer of a CString after the destruction of the CString. The destructor is zeroing that string (possibly because I am running in debug mode) so the net result is not correct.

My symptoms of this are usually that the unit test simply stops without crashing, though sometimes I seem to get a breaking error in the DIR_AtomizePrefName call. Not sure if this is an issue during normal operation or not.
Comment 1 Kent James (:rkent) 2012-06-26 09:25:45 PDT
Created attachment 636735 [details] [diff] [review]
Somebody needs to own the prefname storage

The fix is pretty simple, and hopefully non-controversial, so I'll ask for it since I don't have a good workaround.

The fix itself is obvious, but it may cause a feature that currently may not work to suddenly start working, which could have its own side effects. Not sure what those might be.
Comment 2 Kent James (:rkent) 2012-06-28 10:41:24 PDT
Checked in

Note You need to log in before you can comment on or make changes to this bug.