Closed Bug 571728 Opened 12 years ago Closed 12 years ago

Mismatched allocator for SuggestMgr::csconv

Categories

(Core :: Spelling checker, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: jruderman, Assigned: RyanVM)

References

Details

(Keywords: regression, valgrind, Whiteboard: [fixed-in-hunspell-1.2.12])

Attachments

(3 files, 3 obsolete files)

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==
Flags: in-testsuite+
Keywords: valgrind
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
}
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...
Attached patch Patch (v1) (obsolete) — Splinter Review
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #450936 - Flags: review?(Olli.Pettay)
Attachment #450936 - Flags: review?(ryanvm)
Attachment #450936 - Flags: review?(Olli.Pettay)
Attachment #450936 - Flags: review+
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 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.
Attachment #450936 - Flags: review?(ryanvm) → review+
(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.
http://hg.mozilla.org/mozilla-central/rev/b226cd0d0b7c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attached file valgrind output #2
Still hitting this after building from rev b226cd0d0b7c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Followup (obsolete) — Splinter Review
Attachment #450971 - Flags: review?(Olli.Pettay)
You put the hunspell.cpp include inside an ifndef block. I don't that's what you intended to do :)
Attached patch Followup (obsolete) — Splinter Review
Heh, need to get my glasses upgraded! ;-)
Attachment #450971 - Attachment is obsolete: true
Attachment #450974 - Flags: review?(Olli.Pettay)
Attachment #450971 - Flags: review?(Olli.Pettay)
Jesse, if it's not too much trouble, you can try the followup patch to see if that fixes it?
(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.
I think so.  Tested with a simple contenteditable page.
smaug: ping?
Attachment #450974 - Flags: review?(Olli.Pettay) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/211153ce7d84
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a5 → mozilla1.9.3a6
Whiteboard: [fixed-in-hunspell-1.2.12]
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
}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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?
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?
(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?  ;-)
Yes
Attached patch Like this?Splinter Review
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?
Assignee: ehsan → ryanvm
Status: REOPENED → NEW
Attachment #454219 - Flags: feedback?(ehsan)
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! ;-)
Attachment #454219 - Flags: feedback?(ehsan) → feedback+
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?
Attachment #454219 - Flags: review?(Olli.Pettay)
Yes, this fixes the testcase.  I'll also run the fuzzer a bit just to be sure.
Attachment #454219 - Flags: review?(Olli.Pettay) → review+
Jesse confirmed over IRC no more instances of this bug during fuzzing.
Keywords: checkin-needed
Attachment #450936 - Attachment is obsolete: true
Attachment #450974 - Attachment is obsolete: true
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.
Status: NEW → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3b1 → mozilla1.9.3b2
Depends on: 579649
You need to log in before you can comment on or make changes to this bug.