571728 - Mismatched allocator for SuggestMgr::csconv

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Jesse Ruderman]

I built with:
ac_add_options --disable-jemalloc
ac_add_options --enable-valgrind

==20894== Mismatched free() / delete / delete []
==20894==    at 0x4C27ACB: operator delete[](void*) (vg_replace_malloc.c:409)
==20894==    by 0x5FE6DAE: SuggestMgr::~SuggestMgr() (suggestmgr.cpp:148)
==20894==    by 0x5FE1016: Hunspell::~Hunspell() (hunspell.cpp:103)
==20894==    by 0x5FC6508: mozHunspell::~mozHunspell() (mozHunspell.cpp:114)
==20894==    by 0x5FC6863: mozHunspell::Release() (mozHunspell.cpp:79)
==20894==    by 0x607C642: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.h:456)
==20894==    by 0x60B14E0: FreeServiceContractIDEntryEnumerate(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsCOMPtr.h:975)
==20894==    by 0x607AB47: PL_DHashTableEnumerate (pldhash.c:754)
==20894==    by 0x60B153F: nsComponentManagerImpl::FreeServices() (nsComponentManager.cpp:1776)
==20894==    by 0x6085583: mozilla::ShutdownXPCOM(nsIServiceManager*) (nsXPComInit.cpp:846)
==20894==    by 0x608578A: NS_ShutdownXPCOM_P (nsXPComInit.cpp:758)
==20894==    by 0x555081F: ScopedXPCOMStartup::~ScopedXPCOMStartup() (nsAppRunner.cpp:1077)
==20894==  Address 0x220c28e0 is 0 bytes inside a block of size 768 alloc'd
==20894==    at 0x4C28528: malloc (vg_replace_malloc.c:236)
==20894==    by 0x721E251: moz_xmalloc (mozalloc.cpp:98)
==20894==    by 0x5FD8400: get_current_cs(char const*) (mozalloc.h:236)
==20894==    by 0x5FE6EC1: SuggestMgr::SuggestMgr(char const*, int, AffixMgr*) (suggestmgr.cpp:97)
==20894==    by 0x5FE134D: Hunspell::Hunspell(char const*, char const*, char const*) (hunspell.cpp:97)
==20894==    by 0x5FC62E2: mozHunspell::SetDictionary(unsigned short const*) (mozHunspell.cpp:168)
==20894==    by 0x5FBC923: mozSpellChecker::SetCurrentDictionary(nsAString_internal const&) (mozSpellChecker.cpp:386)
==20894==    by 0x5E8ADFB: nsEditorSpellCheck::SetCurrentDictionary(unsigned short const*) (nsEditorSpellCheck.cpp:465)
==20894==    by 0x5E8BB5C: nsEditorSpellCheck::InitSpellChecker(nsIEditor*, int) (nsEditorSpellCheck.cpp:223)
==20894==    by 0x5FC1B19: mozInlineSpellChecker::SetEnableRealTimeSpell(int) (mozInlineSpellChecker.cpp:733)
==20894==    by 0x5A808B8: nsEditor::SyncRealTimeSpell() (nsEditor.cpp:1282)
==20894==    by 0x59699A2: nsHTMLDocument::EditingStateChanged() (nsHTMLDocument.cpp:3377)
==20894==

Comment 1

[Caolan McNamara]

in hunspell 1.2.11 and in hunspell cvs hunspell/src/hunspell/csutil.cxx reads...

struct cs_info * get_current_cs(const char * es) {
  struct cs_info *ccs;
...
ccs = new cs_info[256];
...
return ccs
}

while http://mxr.mozilla.org/mozilla/source/extensions/spellcheck/hunspell/src/csutil.cpp instead reads...

struct cs_info * get_current_cs(const char * es) {
  struct cs_info *ccs;
...
ccs = (struct cs_info *) malloc(256 * sizeof(cs_info));
...
return ccs
}

Comment 2

[Ryan VanderMeulen [:RyanVM]]

mxr.mozilla.org doesn't seem to be reflecting the landing of 1.2.11 yet. Which is odd because I thought it updated more frequently than that...

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Oh duh, we're looking at the wrong repo.
http://mxr-test.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/src/

Comment 4

[(no longer active)]

Attachment: Patch (v1)

This happens because csutil.cpp #includes "nsCOMPtr.h", which #includes "nscore.h", which #includes our mozalloc headers, so operator new[] as used in that file is our version of that operator, but operator delete[] as used in suggestmgr.cpp is the one provided by libstdc++.

#including "nscore.h" in suggestmgr.cpp should be enough to take care of this problem.

Comment 5

[(no longer active)]

I mistakenly thought that Smaug r+'ed it and it's ready to go, so I landed it:

http://hg.mozilla.org/mozilla-central/rev/3224dabf012e

And immediately backed it out:

http://hg.mozilla.org/mozilla-central/rev/2d156d5c7450

Sorry for polluting the project history :(

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 450936 [details] [diff] [review]
Patch (v1)

If smaug is OK with it, I'm OK with it. Jesse, if you confirm that this fixes it, we can get it upstreamed.

Comment 7

[(no longer active)]

(In reply to comment #6)
> (From update of attachment 450936 [details] [diff] [review])
> If smaug is OK with it, I'm OK with it. Jesse, if you confirm that this fixes
> it, we can get it upstreamed.

This was a bug in our own MOZILLA_CLIENT #ifdef'ed code, so it doesn't need to be upstreamed.

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b226cd0d0b7c

Comment 9

[Jesse Ruderman]

Attachment: valgrind output #2

Still hitting this after building from rev b226cd0d0b7c.

Comment 10

[(no longer active)]

Attachment: Followup

Comment 11

[Ryan VanderMeulen [:RyanVM]]

You put the hunspell.cpp include inside an ifndef block. I don't that's what you intended to do :)

Comment 12

[(no longer active)]

Attachment: Followup

Heh, need to get my glasses upgraded! ;-)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Jesse, if it's not too much trouble, you can try the followup patch to see if that fixes it?

Comment 14

[(no longer active)]

(In reply to comment #13)
> Jesse, if it's not too much trouble, you can try the followup patch to see if
> that fixes it?

Yes, that would be helpful, because I didn't manage to reproduce the original problem.

Comment 15

[Jesse Ruderman]

I think so.  Tested with a simple contenteditable page.

Comment 16

[(no longer active)]

smaug: ping?

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/211153ce7d84

Comment 18

[Jesse Ruderman]

Still hitting this :(

==21981== Mismatched free() / delete / delete []
==21981==    at 0x4C27ACB: operator delete[](void*) (vg_replace_malloc.c:409)
==21981==    by 0x6A242C4: HashMgr::~HashMgr() (hashmgr.cpp:149)
==21981==    by 0x6A2775D: Hunspell::~Hunspell() (hunspell.cpp:107)
==21981==    by 0x6A04229: mozHunspell::~mozHunspell() (mozHunspell.cpp:114)
==21981==    by 0x6A03AA6: mozHunspell::Release() (mozHunspell.cpp:79)
==21981==    by 0x5DBBB97: nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) (nsCOMPtr.h:456)
==21981==    by 0x6B4BC12: nsCOMPtr_base::assign_with_AddRef(nsISupports*) (nsCOMPtr.cpp:89)
==21981==    by 0x5628228: nsCOMPtr<nsISupports>::operator=(nsISupports*) (nsCOMPtr.h:975)
==21981==    by 0x6BBB175: FreeServiceContractIDEntryEnumerate(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (nsComponentManager.cpp:1776)
==21981==    by 0x6B4933D: PL_DHashTableEnumerate (pldhash.c:754)
==21981==    by 0x6BBB1F5: nsComponentManagerImpl::FreeServices() (nsComponentManager.cpp:1789)
==21981==    by 0x6B62D6E: mozilla::ShutdownXPCOM(nsIServiceManager*) (nsXPComInit.cpp:843)
==21981==    by 0x6B62999: NS_ShutdownXPCOM_P (nsXPComInit.cpp:755)
==21981==    by 0x563861B: ScopedXPCOMStartup::~ScopedXPCOMStartup() (nsAppRunner.cpp:1077)
==21981==    by 0x564164B: XRE_main (nsAppRunner.cpp:3671)
==21981==    by 0x401331: main (nsBrowserApp.cpp:158)
==21981==  Address 0x1abe9440 is 0 bytes inside a block of size 768 alloc'd
==21981==    at 0x4C28528: malloc (vg_replace_malloc.c:236)
==21981==    by 0x853904D: moz_xmalloc (mozalloc.cpp:98)
==21981==    by 0x6A22010: get_current_cs(char const*) (mozalloc.h:238)
==21981==    by 0x6A2627C: HashMgr::load_config(char const*, char const*) (hashmgr.cpp:715)
==21981==    by 0x6A23D15: HashMgr::HashMgr(char const*, char const*, char const*) (hashmgr.cpp:88)
==21981==    by 0x6A27397: Hunspell::Hunspell(char const*, char const*, char const*) (hunspell.cpp:81)
==21981==    by 0x6A0466F: mozHunspell::SetDictionary(unsigned short const*) (mozHunspell.cpp:168)
==21981==    by 0x69EF536: mozSpellChecker::SetCurrentDictionary(nsAString_internal const&) (mozSpellChecker.cpp:386)
==21981==    by 0x67B63C1: nsEditorSpellCheck::SetCurrentDictionary(unsigned short const*) (nsEditorSpellCheck.cpp:446)
==21981==    by 0x67B544B: nsEditorSpellCheck::InitSpellChecker(nsIEditor*, int) (nsEditorSpellCheck.cpp:221)
==21981==    by 0x69F9D86: mozInlineSpellChecker::SetEnableRealTimeSpell(int) (mozInlineSpellChecker.cpp:733)
==21981==    by 0x6074EC1: nsEditor::SyncRealTimeSpell() (nsEditor.cpp:1269)
==21981==    by 0x5E40CE6: nsHTMLDocument::EditingStateChanged() (nsHTMLDocument.cpp:3390)
==21981==    by 0x5E3F01A: nsHTMLDocument::MaybeEditingStateChanged() (nsHTMLDocument.cpp:2992)
==21981==    by 0x5E3F093: nsHTMLDocument::EndUpdate(unsigned int) (nsHTMLDocument.cpp:3005)
==21981==    by 0x60F8C93: nsHtml5TreeOpExecutor::EndDocUpdate() (nsHtml5TreeOpExecutor.h:253)
==21981==    by 0x60F7DE0: nsHtml5TreeOpExecutor::StartLayout() (nsHtml5TreeOpExecutor.cpp:657)
==21981==    by 0x60F419F: nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) (nsHtml5TreeOperation.cpp:641)
==21981==    by 0x60F75F1: nsHtml5TreeOpExecutor::RunFlushLoop() (nsHtml5TreeOpExecutor.cpp:485)
==21981==    by 0x60FEF77: nsHtml5ExecutorFlusher::Run() (nsHtml5StreamParser.cpp:152)
==21981==    by 0x6BC7EB8: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:547)
==21981==    by 0x6B55B9B: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:250)
==21981==    by 0x6A6F997: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:118)
==21981==    by 0x6C39EE0: MessageLoop::RunInternal() (message_loop.cc:216)
==21981==    by 0x6C39E65: MessageLoop::RunHandler() (message_loop.cc:199)
==21981==    by 0x6C39DF6: MessageLoop::Run() (message_loop.cc:173)
==21981==    by 0x69121E6: nsBaseAppShell::Run() (nsBaseAppShell.cpp:175)
==21981==    by 0x665FCFC: nsAppStartup::Run() (nsAppStartup.cpp:192)
==21981==    by 0x56414E7: XRE_main (nsAppRunner.cpp:3623)
==21981==    by 0x401331: main (nsBrowserApp.cpp:158)
==21981==
{
   <insert_a_suppression_name_here>
   Memcheck:Free
   fun:_ZdaPv
   fun:_ZN7HashMgrD1Ev
   fun:_ZN8HunspellD1Ev
   fun:_ZN11mozHunspellD0Ev
   fun:_ZN11mozHunspell7ReleaseEv
   fun:_ZN13nsCOMPtr_base22assign_assuming_AddRefEP11nsISupports
   fun:_ZN13nsCOMPtr_base18assign_with_AddRefEP11nsISupports
   fun:_ZN8nsCOMPtrI11nsISupportsEaSEPS0_
   fun:_ZL35FreeServiceContractIDEntryEnumerateP12PLDHashTableP15PLDHashEntryHdrjPv
   fun:PL_DHashTableEnumerate
   fun:_ZN22nsComponentManagerImpl12FreeServicesEv
   fun:_ZN7mozilla13ShutdownXPCOMEP17nsIServiceManager
   fun:NS_ShutdownXPCOM_P
   fun:_ZN18ScopedXPCOMStartupD1Ev
   fun:XRE_main
   fun:main
}

Comment 19

[Jesse Ruderman]

Attachment: testcase (requires shutdown)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Looks like hashmgr.cpp needs some ifdef loving too. I'm going to go through the rest of them tonight to see what other files may need it instead of continuing to do this piecemeal.

Comment 21

[(no longer active)]

(In reply to comment #20)
> Looks like hashmgr.cpp needs some ifdef loving too. I'm going to go through the
> rest of them tonight to see what other files may need it instead of continuing
> to do this piecemeal.

Well, I used mxr to search for csconv, and I can swear that hashmgr.cpp was not among the listed files when I wrote the followup patch!  Do you have a better approach in mind?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

I was thinking we'd need to add it to any files containing new and delete. Or at least the ones that include csutil.hxx? I don't know this stuff well enough to say it with any confidence, but might that work?

Comment 23

[(no longer active)]

(In reply to comment #22)
> I was thinking we'd need to add it to any files containing new and delete. Or
> at least the ones that include csutil.hxx? I don't know this stuff well enough
> to say it with any confidence, but might that work?

Well, technically we don't need to add nscore.h to *every* file including new and delete.  The only thing which matters is that the new and delete versions we use on the same variable match.  But including it in a header which is included throughout hunspell is probably the safest bet.  Were you going to submit a patch?  ;-)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Yes

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Attachment: Like this?

This patch reverts the earlier changes to the various hunspell cpp files and instead adds the ifdef to csutil.hxx. Does this sound sane, Ehsan?

Comment 26

[(no longer active)]

Comment on attachment 454219 [details] [diff] [review]
Like this?

It does!

I was going to r+ it right away, but I've screwed this up so much that I would prefer you get another pair of eyes to look at it! ;-)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 454219 [details] [diff] [review]
Like this?

Olli, can you do the honors?

Also, Jesse, can you confirm that this fixes it for good?

Comment 28

[Jesse Ruderman]

Yes, this fixes the testcase.  I'll also run the fuzzer a bit just to be sure.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Jesse confirmed over IRC no more instances of this bug during fuzzing.

Comment 30

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/93fabb73299b

not sure about target milestone, the bug is long and has multiple landings, please set it correctly.