js_FinalizeStringRT dies with multi-threaded app

VERIFIED FIXED

Status

()

--
major
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: MikeM, Assigned: jorendorff)

Tracking

({testcase, verified1.9.1})

Trunk
x86
All
testcase, verified1.9.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
When running with JIT enabled and with about 10 threads I hit a GC problem.
js_FinalizeStringRT() toss an error and dies:

jsstr.cpp line 3003 tosses this assert:
JS_ASSERT(rt->unitStrings[*chars] == str);

The value of the two things being compared is:
-----
rt->unitStrings[*chars]	0x06dc1ac8 {length=1 u={...} }	JSString *
  length	1	unsigned int
  u	{chars=0x04d27768 "H" base=0x04d27768 }	JSString::<unnamed-tag>
------
str	0x06dc1b98 {length=1 u={...} }	JSString *
  length	1	unsigned int
  u	{chars=0x04d27768 "H" base=0x04d27768 }	JSString::<unnamed-tag>
------

Seems that the strings are equal as far as value but their pointer addresses are not.

CC'ing those that are smarter than me.

P.S.  GC_ZEAL with level 2 is clean for me now.  I've got no rooting bugs that I know of.

Several threads are waiting for JS_AWAIT_GC_DONE()

Call stack for assert thread is:
--------------------------------
JS_Assert(const char * s=0x00663b44, const char * file=0x00663b38, int ln=3003)  Line 59	C++
js_FinalizeStringRT(JSRuntime * rt=0x04bf5ba8, JSString * str=0x06dc1b98, int type=-2, JSContext * cx=0x04d17c68)  Line 3003 + 0x2d bytes	C++
js_GC(JSContext * cx=0x04d17c68, JSGCInvocationKind gckind=GC_LAST_DITCH)  Line 3631 + 0x18 bytes	C++
js_NewGCThing(JSContext * cx=0x04d17c68, unsigned int flags=2, unsigned int nbytes=8)  Line 1855 + 0xb bytes	C++
js_NewString(JSContext * cx=0x04d17c68, unsigned short * chars=0x056468e8, unsigned int length=2)  Line 2824 + 0xd bytes	C++
js_ConcatStrings(JSContext * cx=0x04d17c68, JSString * left=0x06dc1ac8, JSString * right=0x06dc1ac8)  Line 169 + 0x11 bytes	C++
js_Interpret(JSContext * cx=0x04d17c68)  Line 3798 + 0x15 bytes	C++
js_Execute(JSContext * cx=0x04d17c68, JSObject * chain=0x06da1820, JSScript * script=0x04d0dd70, JSStackFrame * down=0x00000000, unsigned int flags=0, int * result=0x07b3f4e8)  Line 1566 + 0x9 bytes	C++
JS_ExecuteScript(JSContext * cx=0x04d17c68, JSObject * obj=0x06da1820, JSScript * script=0x04d0dd70, int * rval=0x07b3f4e8)  Line 5141 + 0x19 bytes	C++
------------------------

Mike M.
(Reporter)

Comment 1

10 years ago
I reproduced the bug with a different call stack today:

JS_Assert(const char * s=0x00664f24, const char * file=0x00664f18, int ln=3003)  Line 59	C++
js_FinalizeStringRT(JSRuntime * rt=0x04bfec48, JSString * str=0x071c1a90, int type=-2, JSContext * cx=0x04d1db18)  Line 3003 + 0x2d bytes	C++
js_GC(JSContext * cx=0x04d1db18, JSGCInvocationKind gckind=GC_LAST_DITCH)  Line 3631 + 0x18 bytes	C++
RefillDoubleFreeList(JSContext * cx=0x04d1db18)  Line 2093 + 0xb bytes	C++
js_NewDoubleInRootedValue(JSContext * cx=0x04d1db18, double d=0.00000000000000000, int * vp=0x04d02a68)  Line 2196 + 0x9 bytes	C++
js_ReplenishReservedPool(JSContext * cx=0x04d1db18, JSTraceMonitor * tm=0x04cfb37c)  Line 1516 + 0x15 bytes	C++
js_MonitorLoopEdge(JSContext * cx=0x04d1db18, unsigned int & inlineCallCount=2)  Line 4170 + 0x20 bytes	C++
js_Interpret(JSContext * cx=0x04d1db18)  Line 3714 + 0x530 bytes	C++
js_Execute(JSContext * cx=0x04d1db18, JSObject * chain=0x06ee1000, JSScript * script=0x055b7460, JSStackFrame * down=0x00000000, unsigned int flags=0, int * result=0x06d5f4e8)  Line 1566 + 0x9 bytes	C++
JS_ExecuteScript(JSContext * cx=0x04d1db18, JSObject * obj=0x06ee1000, JSScript * script=0x055b7460, int * rval=0x06d5f4e8)  Line 5141 + 0x19 bytes	C++
(Assignee)

Comment 2

10 years ago
gczeal(2);

function f() {
    var s;
    for (var i = 0; i < 9999; i++)
        s = 'a' + String(i)[3] + 'b';
    return s;
}

print(scatter([f, f, f, f]));
(Assignee)

Updated

10 years ago
Assignee: general → jorendorff
(Assignee)

Comment 3

10 years ago
Created attachment 363396 [details] [diff] [review]
v1

The assertion here is bogus, so this bug is harmless in opt builds.

It happens when two threads get in js_GetUnitStringForChar with the same `c` at the same time.  The cache is empty.  Both threads create a string with u.chars pointing to the same character.

There is locking around the unit string cache fill, which breaks the tie; the assertion hits when we GC the string that was *not* cached.

Could just delete the assertion, but I like this fractionally better... either way is fine with me.
Attachment #363396 - Flags: review?(igor)

Updated

10 years ago
Attachment #363396 - Flags: review?(igor) → review+

Comment 4

10 years ago
Comment on attachment 363396 [details] [diff] [review]
v1

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

This part is from the wrong patch ;)

>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
>@@ -2742,16 +2742,20 @@ js_GetUnitStringForChar(JSContext *cx, j
> 
>         cp = UNIT_STRING_SPACE_RT(rt);
>         str = js_NewString(cx, cp + 2 * c, 1);
>         if (!str)
>             return NULL;
>         JS_LOCK_GC(rt);
>         if (!rt->unitStrings[c])
>             rt->unitStrings[c] = str;
>+#ifdef DEBUG
>+        else
>+            JSFLATSTR_INIT(str, NULL, 0);  /* Avoid later assertion. See bug 479381. */

Style nit: in-line comments starts from a small letter and do need the final period. So use:

/* avoid later assertion, bug 479381 */

Comment 5

10 years ago
(In reply to comment #4)
> Style nit: in-line comments starts from a small letter and do need the final
> period. So use:
> 
> /* avoid later assertion, bug 479381 */

Odd, I don't remember seeing sentence-fragment comments in SpiderMonkey source before; I'd have thought /* Avoid later assertion; see bug 479381. */ would be the most correct style.

Comment 6

10 years ago
(In reply to comment #5)
> Odd, I don't remember seeing sentence-fragment comments in SpiderMonkey source
> before; 

They are mostly in headers, for example, see http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.h#394 .

Comment 7

10 years ago
Ah; most of those seem to be descriptions sans verbs, tho (although there are a fair number in that file that do have periods at end) -- add in a verb and I think that ups the caps+period use percentage considerably.
(Assignee)

Comment 8

10 years ago
Is it possible this style consistency thing can be taken too far?

http://hg.mozilla.org/tracemonkey/rev/03fc512bd2a2
Summary: js_FinalizeStringRT dies with multi-threaded app and JIT → js_FinalizeStringRT dies with multi-threaded app
Whiteboard: fixed-in-tracemonkey
There is a style rule for "on the side" comments, meant to support their brevity: sentence fragments, no capitalization, no full stop. Period ;-).

If you want to write a full sentence, it probably should go before the statement being commented upon. Imagine growing the "on the side" (after) comment to overflow to multiple lines and turning into a paragraph of many sentences...

/be

Comment 10

10 years ago
http://hg.mozilla.org/mozilla-central/rev/03fc512bd2a2
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite?
Keywords: testcase

Comment 12

10 years ago
http://hg.mozilla.org/tracemonkey/rev/9ee7860a3386

/cvsroot/mozilla/js/tests/js1_8/extensions/regress-479381.js,v  <--  regress-479381.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+

Comment 13

10 years ago
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.