445818 - Crash in Javascript on N810 (arm).

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Oleg Romashin (:romaxa)]

Latest trunk, arm, N810.
Visit some google service pages mail/maps/documents

5/10 it will crash in:
#0  js_Dequeue (tl=0xb99dc) at js/src/jslock.cpp:993
#1  0x41e8d06c in js_AtomizeString (cx=0xb124d8, str=0xbed41c98, flags=8) at js/src/jsatom.cpp:704
#2  0x41e8d23c in js_AtomizeChars (cx=0x0, chars=0x0, length=760284, flags=0) at js/src/jsatom.cpp:756
#3  0x41eee93c in js_GetToken (cx=0xb124d8, ts=0xbed42e80) at js/src/jsscan.cpp:1258
#4  0x41ef0478 in js_MatchToken (cx=0x0, ts=0xbed42e80, tt=TOK_YIELD) at js/src/jsscan.cpp:1999
#5  0x41ed8304 in AssignExpr (cx=0xb124d8, ts=0xbed42e80, tc=0xbed422f8) at js/src/jsparse.cpp:3701
#6  0x41ede854 in Variables (cx=0xb124d8, ts=0xbed42e80, tc=0xbed422f8) at js/src/jsparse.cpp:3637
#7  0x41edac94 in Statement (cx=0xb124d8, ts=0xbed42e80, tc=0xbed422f8) at js/src/jsparse.cpp:3239
#8  0x41edc1a4 in Statements (cx=0xb124d8, ts=0xbed42e80, tc=0xbed422f8) at js/src/jsparse.cpp:1455
#9  0x41edc33c in FunctionBody (cx=0xb124d8, ts=0xbed42e80, tc=0xbed422f8) at js/src/jsparse.cpp:851
#10 0x41edc6a0 in FunctionDef (cx=0xb124d8, ts=0xbed42e80, tc=0xbed42710, lambda=8) at js/src/jsparse.cpp:1295
#11 0x41edd5a0 in PrimaryExpr (cx=0xb124d8, ts=0xbed42e80, tc=0xbed42710, tt=TOK_FUNCTION, afterDot=0) at js/src/jsparse.cpp:5352
....
#58 0x41ed8328 in AssignExpr (cx=0xb124d8, ts=0xbed42e80, tc=0xbed42cf8) at js/src/jsparse.cpp:3818
#59 0x41ed86f4 in Expr (cx=0xb124d8, ts=0xbed42e80, tc=0xbed42cf8) at js/src/jsparse.cpp:3662
#60 0x41ed9bb4 in Statement (cx=0xb124d8, ts=0xbed42e80, tc=0xbed42cf8) at js/src/jsparse.cpp:3454
#61 0x41edf540 in js_CompileScript (cx=0xb124d8, obj=0xbed42cf8, principals=0xb99dc, tcflags=2048, chars=0x49589010, length=1102854124, file=0x41edf540, 
    filename=0x41bc37ec "°\022\203@4¦\203@T=¹A°ÚºAT=¹AT=¹A\\\021\203@ð\005»A@ȹAT=¹AT=¹A̬ºAT=¹AT=¹AT=¹AT=¹AT=¹AT=¹A¸ÞºA\204v\177@T=¹A0$ºAT=¹A\200ÞºAøÕ¹Að\205\201@T=¹AÄ3ºA\214\017\203@T=¹A\f*ºA$¢¹At¨ºAT=¹AT=¹AT=¹AT=¹AT=¹AT=¹A¼#»Ax[¼A¬:¼A`¸y@\034]¼AÌ\023\002", lineno=11609348)
    at js/src/jsparse.cpp:611
(gdb) frame 0
#0  js_Dequeue (tl=0xb99dc) at /home/romaxa/microbcomponent/hg/mozilla/js/src/jslock.cpp:993
993	    PR_Lock(fl->slock);
(gdb) l 
988	    JSFatLock *fl = tl->fat;
989	    PRStatus stat;
990	
991	    JS_ASSERT(fl != NULL);
992	    JS_ASSERT(fl->susp > 0);
993	    PR_Lock(fl->slock);
994	    js_UnlockGlobal(tl);
995	    stat = PR_NotifyCondVar(fl->svar);
996	    JS_ASSERT(stat != PR_FAILURE);
997	    PR_Unlock(fl->slock);
(gdb) p *tl 
$3 = {owner = 0, fat = 0x0}

Comment 1

[Igor Bukanov]

It seems this is a regression from the bug 444076. Can you check if USE_ARM_KUSER is defined during the build of js/src/jslock.cpp? 

Comment 2

[Igor Bukanov]

Attachment: v1

I was wrong about using generic __sync_bool_compare_and_swap available in GCC. Apparently one cannot trust the function as this bug demonstrates. When USE_ARM_KUSER is not defined GCC for ARM may either miscompile or generate the wrong code for __sync_bool_compare_and_swap.

So the patch does not trust GCC __sync_bool_compare_and_swap and just code it explicitly for x86-64.

Comment 3

[Oleg Romashin (:romaxa)]

Yep, with this patch it works without crashes.

Comment 4

[Igor Bukanov]

Attachment: v1 as hg bundle

Comment 5

[Anton Rogaynis (antonr)]

Still crashes at the same place:
#0  js_Dequeue (tl=0xbf30c) at js/src/jslock.cpp:1001
#1  0x41cad06c in js_AtomizeString (cx=0x6c0b88, str=0xbee0e670, flags=8) at js/src/jsatom.cpp:704
#2  0x41cad23c in js_AtomizeChars (cx=0x0, chars=0x0, length=783116, flags=0)
(gdb) frame 0
#0  js_Dequeue (tl=0xbf30c) at /home/bifh4/diablo-arm-prereleased.gcc34qemu.distcc/work/microb-engine-20080616/build-tree/mozilla/js/src/jslock.cpp:1001
1001	    PR_Lock(fl->slock);
(gdb) p *tl
$7 = {owner = 0, fat = 0x0}
(gdb) l
996	    JSFatLock *fl = tl->fat;
997	    PRStatus stat;
998	
999	    JS_ASSERT(fl != NULL);
1000	    JS_ASSERT(fl->susp > 0);
1001	    PR_Lock(fl->slock);
1002	    js_UnlockGlobal(tl);
1003	    stat = PR_NotifyCondVar(fl->svar);
1004	    JS_ASSERT(stat != PR_FAILURE);
1005	    PR_Unlock(fl->slock);

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> Still crashes at the same place:
> #0  js_Dequeue (tl=0xbf30c) at js/src/jslock.cpp:1001

Do you mean it crashes with the patch?

Comment 7

[Anton Rogaynis (antonr)]

(In reply to comment #6)
> (In reply to comment #5)
> > Still crashes at the same place:
> > #0  js_Dequeue (tl=0xbf30c) at js/src/jslock.cpp:1001
> 
> Do you mean it crashes with the patch?
Yep, it crashes with the patch. 

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Still crashes at the same place:
> > > #0  js_Dequeue (tl=0xbf30c) at js/src/jslock.cpp:1001
> > 
> > Do you mean it crashes with the patch?
> Yep, it crashes with the patch. 

Hm, but that mean that USE_ARM_KUSER is defined when js/src/jslock.cpp is compiled and neither this patch not the bug 444076 shpould not cause the regression. Can you check USE_ARM_KUSER? 

Comment 9

[Oleg Romashin (:romaxa)]

Yes, we are using --with-arm-kuser option, because  it N8xx - arm + kernel
and USE_ARM_KUSER=1

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Yes, we are using --with-arm-kuser option, because  it N8xx - arm + kernel
> and USE_ARM_KUSER=1
> 

Then neither bug 444076 nor the patch can any effects on your crash since the code changes does not touch the code compiled on ARM.

Could you try to compile with -O0 to see if the crash would happen with no optimizations?

Comment 11

[Oleg Romashin (:romaxa)]

I have tested with -O0 already and it does not crash....
At least I was testing during 20 minutes and did not get any crash.

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> I have tested with -O0 already and it does not crash....
> At least I was testing during 20 minutes and did not get any crash.
> 

Then this looks like a compiler bug perhaps triggered by changes from the bug 440184 that added an extra level of inlining. Can you try with normal optimization level but with ThinkLock()/ThinkUnlock() changed  (they defined in two places, the changes has to go to functions defined right after js_Dequeue) from static JS_INLINE to just static.

If that would not help, can you try to remove both static and JS_INLINE?

Comment 13

[Anton Rogaynis (antonr)]

tried to remove JS_INLINE only and still crashes in the same place.
I'll try to remove both.

Comment 14

[Anton Rogaynis (antonr)]

removing both did not help.

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> removing both did not help.
> 

One more idea: could you try to remove static JS_INLINE from NativeCompareAndSwap defined under #elif defined(USE_ARM_KUSER)  ? And if that would not help, try to add __attributes__((noinline)) to NativeCompareAndSwap.

Comment 16

[Oleg Romashin (:romaxa)]

Hmm, ok I will try that... 

Igor, do you have any ideas which simple js testcase can reproduce this problem?
Usually it crashes on gmail ui=1/2, but
I tiered to load google services every time.... :(

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> Hmm, ok I will try that... 
> 
> Igor, do you have any ideas which simple js testcase can reproduce this
> problem?

Something like:

function test() {
    var obj = {};
    for (var i = 0; i != 42*42*42; ++i) {
        obj["a"+i] = 1;
        var tmp = { "a"+i: 2 }; 
    }
}

test();

But given the bug nature I suspect one needs at least 2 threads to trigger it. If you can compile and run thread-safe js shell, then you can try the above script in the shell replacing the "test()" invocation via:

scatter([test, test, test, test]);

That would run the function test on 4 threads. 

Comment 18

[Igor Bukanov]

Actually if you can run the thread-safe shell, try something like:

var array = [{}, {}, {}, {}];

function test() {
    for (var i = 0; i != 42*42*42; ++i) {
        var obj = array[i % array.length];
        obj["a"+i] = 1;
        var tmp = { "a"+i: 2 }; 
    }
}

scatter([test, test, test, test]);


This will makes 4 threads to access the same objects over and over.

Comment 19

[Oleg Romashin (:romaxa)]

var tmp = { "a"+i: 2 }; 
typein:1: SyntaxError: missing : after property id:
typein:1: var tmp = { "a"+i: 2 };

Comment 20

[Igor Bukanov]

(In reply to comment #19)
> var tmp = { "a"+i: 2 }; 
> typein:1: SyntaxError: missing : after property id:
> typein:1: var tmp = { "a"+i: 2 };

Sorry for that mental blunder. replace that var tmp = { "a"+i: 2 }; with

var tmp = {};
tmp["a"+i] = 2;

Comment 21

[Oleg Romashin (:romaxa)]

> would not help, try to add __attributes__((noinline)) to NativeCompareAndSwap.
> 
After ((noinline)) I did not found any crash.

PS: testcase does not reproduce crash...

Comment 22

[Igor Bukanov]

I filed the bug 446169 to implement noinline workaround as the patch in this bug fix at least the linking problem when USE_ARM_KUSER is not defined.

Comment 23

[Igor Bukanov]

I pushed the patch from the comment 2: http://hg.mozilla.org/mozilla-central/index.cgi/rev/4e56cfc08e5d

Comment 24

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8/extensions/regress-445818.js,v  <--  regress-445818.js
initial revision: 1.1

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