415474 - Assert in js_AtomizeString with Trunk and 2+ threads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Moening]

After pulling down the latest truck I know have the following assert anytime I run two or more threads against JS.

JS_ASSERT(JSSTRING_IS_ATOMIZED(key));

The assert hits from all kinds of different places in the code with various strings. ex:  js_InitCommonAtoms() etc..

Nothing on my end has changed which would cause this to occur.
I'm running with thin locks now per bug #353962 but I have no idea if this is related.  Lots of changes in the last few months. GC changes too.  Not sure where to look next...???  Last pull of trunk was December 07.

Comment 1

[Mike Moening]

Tried defining JS_USE_ONLY_NSPR_LOCKS but ASSERT still occurs.
Very easy to re-produce.
  
Any changes to jsatom.c that might be suspect?

Comment 2

[Mike Moening]

Patch in bug 412340 made on ~1/24/2008 seems to be the cause of this.  Adding notes to bug #412340 too.

Comment 3

[Igor Bukanov]

(In reply to comment #1)
> Tried defining JS_USE_ONLY_NSPR_LOCKS but ASSERT still occurs.
> Very easy to re-produce.

Could you give a stack trace that hits the assert?

Comment 4

[Mike Moening]

There is no pattern.  Any string atomization from pretty much anywhere.  Standard class initialization does it, aliasing a property name, etc...  Usually there is 1 thread doing js_AtomizeString() while the other threads are waiting on a lock to do the same thing.
I'll send you a stack trace something tommorrow if you think you really need it.

Can you give me more info on what this ASSERT actually means?
I have no clue what "atomization" is or what it does.
Then I could debug it further myself too.

Comment 5

[Brendan Eich [:brendan]]

Seems clear key needs to be reloaded from entry before that goto finish. Patch in a trice.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: fix

Comment 7

[Brendan Eich [:brendan]]

MikeM: in comment 4 you talk about threads racing to atomize strings. Sounds like you have a flurry of JS_InitStandardClasses action among multiple threads as they start up -- true? You might want to use lazy standard class init instead.

/be

Comment 8

[Igor Bukanov]

Comment on attachment 301185 [details] [diff] [review]
fix

Sigh. It is so simple. But at least it proves that asserts works.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 301185 [details] [diff] [review]
fix

Safe fix, needs to get in sooner to avoid the trouble MikeM saw (possible in Firefox with anti-phishing? Evidence from bug 353962 shows contention for atom table lock, so if the main thread and anti-phishing thread race to atomize the same string...).

/be

Comment 10

[Mike Schroepfer]

Comment on attachment 301185 [details] [diff] [review]
fix

yea - let's get this for b3 - thanks for the fast work guys.

Comment 11

[Brendan Eich [:brendan]]

Fixed:

js/src/jsatom.c 3.122

/be

Comment 12

[Mike Moening]

Line:

key = (JSString *)ATOM_ENTRY_KEY(entry);

of the fix produces the following warning:

.\jsatom.c(602) : warning C4133: '=' : incompatible types - from 'JSString *' to 'jsdouble *'

Can we fix this too?

Comment 13

[Mike Moening]

Ignore that previous comment.
I applied the fix to the wrong place in the code!
Doh!

Comment 14

[Mike Moening]

Another assert, same function, similar problem, so I re-opened the bug.

if (state->tablegen == gen) {
   JS_ASSERT(entry->keyAndFlags == 0);   <--- ASSERTS here.

Comment 15

[Mike Moening]

This time the stack is coming from a call to JS_DefineConstDoubles() or a call to JS_InitStandardClasses() or others...

Comment 16

[Mike Moening]

In release mode this bug ultimately causes memory corruption and a crash.
Not good.

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #15)
> This time the stack is coming from a call to JS_DefineConstDoubles() or a call
> to JS_InitStandardClasses() or others...

Please be specific -- full stacks up to your embedding code (all the JS code) are helpful.

Igor's out so I'll cover this. Separate bug would be better, as usual.

/be

Comment 18

[Brendan Eich [:brendan]]

Show the value(s) being atomized too, if you can. Thanks.

/be

Comment 19

[Mike Moening]

This one has two threads:

1st threads stack:
-------------------
JS_Assert(const char * s=0x101da8e0, const char * file=0x101da8d4, int ln=682)  Line 59	C
js_AtomizeString(JSContext * cx=0x00db89a0, JSString * str=0x01e3ede4, unsigned int flags=8)  Line 682 + 0x1f bytes	C
js_Atomize(JSContext * cx=0x00db89a0, const char * bytes=0x101ccda4, unsigned int length=2, unsigned int flags=0)  Line 744 + 0x14 bytes	C
JS_AliasProperty(JSContext * cx=0x00db89a0, JSObject * obj=0x013f0820, const char * name=0x101ccda8, const char * alias=0x101ccda4)  Line 3156 + 0x1c bytes	C
js_InitRegExpClass(JSContext * cx=0x00db89a0, JSObject * obj=0x013c2120)  Line 4248 + 0x39 bytes	C
JS_InitStandardClasses(JSContext * cx=0x00db89a0, JSObject * obj=0x013c2120)  Line 1288 + 0xb1 bytes	C
HttpResponse::ExecuteMWServerPageScript(...)  Line 2185 + 0x16 bytes	C++

The string being atomized was "$*"


2nd Threads stack
-------------------
_PR_TIMED_WAIT_SEM(PRSem * sem=0x00e02288, unsigned int ticks=4294967295)  Line 70 + 0xe bytes	C
_PR_WaitCondVar(unsigned int threadId=2680, PRCondVar * cvar=0x00c9b368, PRLock * externalLock=0x00c9b310, unsigned int timeout=4294967295)  Line 84 + 0x22 bytes	C
PRCondVar * cvar=0x00c9b368, unsigned int timeout=4294967295)  Line 179 + 0x17 bytes	C
js_SuspendThread(JSThinLock * tl=0x00d8030c)  Line 940 + 0xe bytes	C
js_Enqueue(JSThinLock * tl=0x00d8030c, long me=14287120)  Line 981 + 0x9 bytes	C
js_Lock(JSThinLock * tl=0x00d8030c, long me=14287120)  Line 1014 + 0xd bytes	C
js_AtomizeString(JSContext * cx=0x00d9b998, JSString * str=0x01d3ed74, unsigned int flags=8)  Line 637 + 0x16 bytes	C
js_Atomize(JSContext * cx=0x00d9b998, const char * bytes=0x101ce840, unsigned int length=6, unsigned int flags=0)  Line 744 + 0x14 bytes	C
JS_DefineFunction(JSContext * cx=0x00d9b998, JSObject * obj=0x013f0dc0, const char * name=0x101ce840, int (JSContext *, JSObject *, unsigned int, long *, long *)* call=0x100fbf80, unsigned int nargs=1, unsigned int attrs=2048)  Line 4361 + 0x1c bytes	C
JS_DefineFunctions(JSContext * cx=0x00d9b998, JSObject * obj=0x013f0dc0, JSFunctionSpec * fs=0x101ce950)  Line 4345 + 0x26 bytes	C
JS_InitClass(JSContext * cx=0x00d9b998, JSObject * obj=0x013c2000, JSObject * parent_proto=0x013f09a0, JSClass * clasp=0x101ce1d0, int (JSContext *, JSObject *, unsigned int, long *, long *)* constructor=0x100fcf80, unsigned int nargs=1, JSPropertySpec * ps=0x101ce7e4, JSFunctionSpec * fs=0x101ce8a0, JSPropertySpec * static_ps=0x00000000, JSFunctionSpec * static_fs=0x00000000)  Line 2734 + 0x35 bytes	C
js_InitArrayClass(JSContext * cx=0x00d9b998, JSObject * obj=0x013c2000)  Line 2161 + 0x29 bytes	C
JS_InitStandardClasses(JSContext * cx=0x00d9b998, JSObject * obj=0x013c2000)  Line 1288 + 0xd bytes	C
HttpResponse::ExecuteMWServerPageScript(...)  Line 2185 + 0x16 bytes	C++

The string being atomized was "concat"

Let me know if you need anything else.
I plan on adding lazy standard class resolving but not until these bugs are fixed first.

Comment 20

[Brendan Eich [:brendan]]

What is *entry in thread 1 frame 1?

Any other threads running, or might have been running and racing?

/be

Comment 21

[Mike Schroepfer]

Moving TM to Beta4 if we get a fix quickly we'll respin for b3 otherwise we'll pickup in B4

Comment 22

[Brendan Eich [:brendan]]

This bug's exact symptom was fixed by its patch; it should remain closed. MikeM has found another bug, a regression from the patch for bug 386265 which I filed as bug 415721, and which I will patch separately. It too is blocking1.9+.

/be