Last Comment Bug 701387 - RegExp related GC Crash [@ JSRuntime::updateMallocCounter] (Memory corruption)
: RegExp related GC Crash [@ JSRuntime::updateMallocCounter] (Memory corruption)
Status: VERIFIED FIXED
[sg:critical][qa-] js-triage-needed
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla11
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
Depends on:
Blocks: langfuzz 634654
  Show dependency treegraph
 
Reported: 2011-11-10 08:20 PST by Christian Holler (:decoder)
Modified: 2013-03-11 08:54 PDT (History)
8 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
unaffected
-
unaffected
-
unaffected
+
fixed
unaffected
unaffected


Attachments
shell testcase, unpack, chdir and read README (1.46 KB, application/x-compressed-tar)
2011-11-10 08:20 PST, Christian Holler (:decoder)
no flags Details
Lookup cache again across possible GC. (8.09 KB, patch)
2011-11-10 14:21 PST, Chris Leary [:cdleary] (not checking bugmail)
jwalden+bmo: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2011-11-10 08:20:54 PST
Created attachment 573522 [details]
shell testcase, unpack, chdir and read README

The attached test crashes on mozilla-central revision (options -m -n). This test was very hard to reduce and still requires the driver to run (see README in the tarball for running instructions). It also switched between error messages of all kind (asserts, crashes, glibc aborts... just the guru meditation was missing to make the set complete).


Backtrace from GDB:

Program received signal SIGSEGV, Segmentation fault.
0x000000000041a3ea in JSRuntime::updateMallocCounter (this=0xb25d10, nbytes=24) at ../../jscntxt.h:799
799             ptrdiff_t newCount = gcMallocBytes - ptrdiff_t(nbytes);
(gdb) bt
#0  0x000000000041a3ea in JSRuntime::updateMallocCounter (this=0xb25d10, nbytes=24) at ../../jscntxt.h:799
#1  0x000000000041a2fd in JSRuntime::malloc_ (this=0xb25d10, bytes=24, cx=0x0) at ../../jscntxt.h:735
#2  0x000000000046ecc7 in js::RuntimeAllocPolicy::malloc_ (this=0xb2e9f0, bytes=24) at /srv/repos/mozilla-central/js/src/jscntxt.h:2455
#3  0x00000000004701ae in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::createTable (alloc=..., capacity=1) at ./dist/include/js/HashTable.h:342
#4  0x0000000000596472 in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::changeTableSize (this=0xb2e9f0, deltaLog2=0) at ./dist/include/js/HashTable.h:556
#5  0x00000000006538ab in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::add (this=0xb2e9f0, p=...) at ./dist/include/js/HashTable.h:712
#6  0x0000000000653527 in js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::add (this=0xb2e9f0, p=..., pentry=0x7fffffffb548) at ./dist/include/js/HashTable.h:735
#7  0x0000000000652ea5 in js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::add (this=0xb2e9f0, p=..., k=@0x7fffffffb5e8, 
    v=@0x7fffffffb5f8) at ./dist/include/js/HashTable.h:1053
#8  0x0000000000652320 in js::RegExpPrivate::create (cx=0xb1eef0, source=0x7ffff6228340, flags=js::StickyFlag, ts=0x0) at ../vm/RegExpObject-inl.h:227
#9  0x000000000064ffaa in js::RegExpObject::makePrivate (this=0x7ffff6007340, cx=0xb1eef0) at /srv/repos/mozilla-central/js/src/vm/RegExpObject.cpp:237
#10 0x000000000058ff86 in js::RegExpObject::getOrCreatePrivate (this=0x7ffff6007340, cx=0xb1eef0) at /srv/repos/mozilla-central/js/src/vm/RegExpObject.h:161
#11 0x000000000064fcc8 in js::RegExpObjectBuilder::clone (this=0x7fffffffb750, other=0x7ffff6007340, proto=0x7ffff60071a8)
    at /srv/repos/mozilla-central/js/src/vm/RegExpObject.cpp:152
#12 0x0000000000650c0b in js_CloneRegExpObject (cx=0xb1eef0, obj=0x7ffff6007340, proto=0x7ffff60071a8) at /srv/repos/mozilla-central/js/src/vm/RegExpObject.cpp:508
#13 0x00000000004f2bf6 in js::Interpret (cx=0xb1eef0, entryFrame=0x7ffff63fb140, interpMode=js::JSINTERP_NORMAL) at /srv/repos/mozilla-central/js/src/jsinterp.cpp:4170

Valgrind message (does not crash in valgrind):

==14402== Invalid write of size 8
==14402==    at 0x652ECC: js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::add(js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::AddPtr&, JSAtom* const&, js::RegExpPrivate* const&) (HashTable.h:1056)
==14402==    by 0x65231F: js::RegExpPrivate::create(JSContext*, JSLinearString*, js::RegExpFlag, js::TokenStream*) (RegExpObject-inl.h:227)
==14402==    by 0x64FFA9: js::RegExpObject::makePrivate(JSContext*) (RegExpObject.cpp:237)
==14402==    by 0x58FF85: js::RegExpObject::getOrCreatePrivate(JSContext*) (RegExpObject.h:161)
==14402==    by 0x64FCC7: js::RegExpObjectBuilder::clone(js::RegExpObject*, js::RegExpObject*) (RegExpObject.cpp:152)
==14402==    by 0x650C0A: js_CloneRegExpObject(JSContext*, JSObject*, JSObject*) (RegExpObject.cpp:508)
==14402==    by 0x4F2BF5: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:4170)
==14402==  Address 0x5e5e548 is 328 bytes inside a block of size 768 free'd
==14402==    at 0x4C282ED: free (vg_replace_malloc.c:366)
==14402==    by 0x40382B: js_free (Utility.h:183)
==14402==    by 0x4114F3: js::Foreground::free_(void*) (Utility.h:597)
==14402==    by 0x41A3D3: JSRuntime::free_(void*) (jscntxt.h:770)
==14402==    by 0x46ECEF: js::RuntimeAllocPolicy::free_(void*) (jscntxt.h:2457)
==14402==    by 0x4702D5: js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::destroyTable(js::RuntimeAllocPolicy&, js::detail::HashTableEntry<js::HashMapEntry<JSAtom*, js::RegExpPrivate*> >*, unsigned int) (HashTable.h:354)
==14402==    by 0x46FEBD: js::detail::HashTable<js::HashMapEntry<JSAtom*, js::RegExpPrivate*>, js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::~HashTable() (HashTable.h:415)
==14402==    by 0x46F50B: js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>::~HashMap() (jsprvtd.h:200)
==14402==    by 0x46F530: void JSRuntime::delete_<js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy> >(js::HashMap<JSAtom*, js::RegExpPrivate*, js::DefaultHasher<JSAtom*>, js::RuntimeAllocPolicy>*) (in /srv/repos/mozilla-central/js/src/debug64/shell/js)
==14402==    by 0x46B731: js::ThreadData::purgeRegExpPrivateCache(JSRuntime*) (jscntxt.cpp:178)
==14402==    by 0x46B816: js_PurgeThreads_PostGlobalSweep(JSContext*) (jscntxt.cpp:358)
==14402==    by 0x4AAA23: GCCycle(JSContext*, JSCompartment*, JSGCInvocationKind) (jsgc.cpp:2919)

This could be another manifestation of bug 701332, but I am not sure on that. S-s and sg:critical because of invalid writes and obvious memory corruption.
Comment 1 Christian Holler (:decoder) 2011-11-10 08:22:25 PST
Missed the revision, this is m-c c60535115ea1.
Comment 2 Chris Leary [:cdleary] (not checking bugmail) 2011-11-10 11:39:49 PST
Wow, nice find, Christian. Looking into it now.
Comment 3 Chris Leary [:cdleary] (not checking bugmail) 2011-11-10 13:27:21 PST
Note: 64b only.
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2011-11-10 14:09:39 PST
Only 11 should be affected, because the RegExpPrivateCache landed there. Bug 634654.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-10 14:19:26 PST
Thanks Chris, flags updated.
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-11-10 14:21:53 PST
Created attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

The cache can totally disappear across the uncached RegExpPrivate creation.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-10 16:07:18 PST
Comment on attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

Review of attachment 573642 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/RegExpObject-inl.h
@@ +197,5 @@
> +    if (!cache)
> +        return false;
> +
> +    RegExpPrivateCache::Ptr p = cache->lookup(atom);
> +    if (p) {

if (RegExpPrivateCache::Ptr p = ...)

@@ +220,5 @@
> +    if (!cache)
> +        return false;
> +
> +    RegExpPrivateCache::AddPtr addPtr = cache->lookupForAdd(atom);
> +    if (addPtr) {

You could even do if (REPC::AP addPtr = ...) here, because addPtr is exposed to both arms of the if, but maybe that looks weird.  I personally am fine with it.

@@ +255,5 @@
>       * |RegExpPrivate|'s deallocation decref will first cause it to
>       * remove itself from the cache.
>       */
>  
> +    JSAtom *sourceAtom = &source->asAtom();

Keeping this as a reference would make for nicer interfaces, I think, with them defining the argument as non-null.

@@ +267,5 @@
>  
> +    /*
> +     * Note: allocations performed by this call may cause a garbage collection, so we have to
> +     * re-lookup the cache (and inside the cache) after the allocation is performed.
> +     */

This is nicely informative.  Yet if the interface as used here doesn't even expose the cache, isn't this comment in the wrong place, and shouldn't it be in cacheInsert or something?

@@ +417,5 @@
>  
> +#ifdef DEBUG
> +    this->~RegExpPrivate();
> +    memset(this, 0xcd, sizeof(*this));
> +    cx->free_(this);

Moar poisoningFree!
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-11-10 21:30:40 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50deaec86e7
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-11-13 13:47:01 PST
https://hg.mozilla.org/mozilla-central/rev/f50deaec86e7
Comment 10 Chris Leary [:cdleary] (not checking bugmail) 2011-11-14 11:36:04 PST
Comment on attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

Sorry jst, I was wrong about what made the merge. The RegExpPrivate cache is indeed in aurora, so 10 is affected.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2011-11-14 17:39:15 PST
Thanks for the correction, Chris!
Comment 12 Chris Leary [:cdleary] (not checking bugmail) 2011-11-14 19:34:42 PST
Comment on attachment 573642 [details] [diff] [review]
Lookup cache again across possible GC.

The RegExpPrivate cache is going to be backed out of Aurora (along with RegExpPrivate laziness), so the aurora flag is no longer necessary.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-02-23 16:46:34 PST
[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.
Comment 14 Chris Leary [:cdleary] (not checking bugmail) 2012-02-26 23:50:00 PST
(In reply to Lukas Blakk [:lsblakk] from comment #13)

Hi Lukas, this does not need to be tracked for ESR due to backout of the RegExpPrivateCache from that release. Thanks!
Comment 15 Gary Kwong [:gkw] [:nth10sd] 2012-02-27 00:22:00 PST
> this does not need to be tracked for ESR due to backout of the
> RegExpPrivateCache from that release. Thanks!

Fixing up flags to reflect that Fx 10 & ESR 10 are not affected, removing from tracking for ESR 10 as well as setting unaffected on appropriate flags.
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2012-02-27 10:47:39 PST
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #15)
> > this does not need to be tracked for ESR due to backout of the
> > RegExpPrivateCache from that release. Thanks!
> 
> Fixing up flags to reflect that Fx 10 & ESR 10 are not affected, removing
> from tracking for ESR 10 as well as setting unaffected on appropriate flags.

thank you Gary!
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-07 14:48:18 PST
Christian, could you give QA any assistance in verifying this fix? I'm currently seeing the following error when running your attached test in a debug shell built from latest-mozilla-beta:

ReferenceError: jsTestDriverEnd is not defined
Comment 18 Christian Holler (:decoder) 2012-03-07 15:00:07 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #17)
> Christian, could you give QA any assistance in verifying this fix? I'm
> currently seeing the following error when running your attached test in a
> debug shell built from latest-mozilla-beta:
> 
> ReferenceError: jsTestDriverEnd is not defined

Did you manage to reproduce the issue in an older shell? The tests I create are not semantically correct most of the time, so error messages such as the one you got here are expected.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-07 15:05:01 PST
No, I wasn't able to reproduce in an early shell. I'm now pulling down the changeset you indicate in comment 1 to see if it reproduces there.
Comment 20 Christian Holler (:decoder) 2012-03-07 15:06:29 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #19)
> No, I wasn't able to reproduce in an early shell. I'm now pulling down the
> changeset you indicate in comment 1 to see if it reproduces there.

Okay. If that does not work for you, just ping me on IRC, and I will try to verify this instead.
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-07 16:11:12 PST
Same error with changeset from comment 1.
Comment 22 Christian Holler (:decoder) 2012-03-07 16:46:06 PST
I wasn't able to reproduce this issue either, even with the changeset specified by myself in comment 1. It's unlikely though that the issue is still present because the whole feature causing it was backed out and fuzzers haven't detected this anymore since then.
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-08 11:17:41 PST
Given comment 22, QA won't be able to verify this fix. If someone is able to reproduce the original issue, please verify the fix in Firefox 11.
Comment 24 Christian Holler (:decoder) 2012-03-23 16:42:21 PDT
I'm marking this verified because we did everything possible to try and verify this but reproduction of the original issue wasn't successful as per comment 22.

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