925312 - Threadsafe opt-only Crash [@ ArrayJoinKernel]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision aa986b6ce882 (threadsafe build, run with --ion-eager):


Array(1024).join(Array(476655))

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Crash trace:

Program received signal SIGSEGV, Segmentation fault.
ArrayJoinKernel<false, StringSeparatorOp> (sb=..., length=1024, obj=(JSObject * const) 0xf6f314a0 [object Array], sepOp=..., cx=0x9079390) at js/src/jsarray.cpp:1049
1049                if (++i != length && !sepOp(cx, sb))
#0  ArrayJoinKernel<false, StringSeparatorOp> (sb=..., length=1024, obj=(JSObject * const) 0xf6f314a0 [object Array], sepOp=..., cx=0x9079390) at js/src/jsarray.cpp:1049
#1  ArrayJoin<false> (args=<synthetic pointer>, cx=0x9079390) at js/src/jsarray.cpp:1124
#2  array_join (vp=0xffffc8a4, argc=1, cx=0x9079390) at js/src/jsarray.cpp:1190
#3  array_join (cx=0x9079390, argc=1, vp=0xffffc8a4) at js/src/jsarray.cpp:1185
#4  0xf729c151 in ?? ()
#5  0x083210b6 in EnterIon (data=..., cx=0x9079390) at js/src/jit/Ion.cpp:2057
#6  js::jit::Cannon (cx=0x9079390, state=...) at js/src/jit/Ion.cpp:2137
#7  0x08090f2c in js::RunScript (cx=0x9079390, state=...) at js/src/vm/Interpreter.cpp:399
eax     0x0     0
ebx     0x903dff4       151248884
ecx     0x3a2f7 238327
edx     0xa2ffe008      -1560289272
esi     0x9079390       151491472
edi     0xe2fff008      -486543352
ebp     0x1     1
esp     0xffffc760      4294952800
eip     0x818faf8 <array_join(JSContext*, unsigned int, JS::Value*)+920>
=> 0x818faf8 <array_join(JSContext*, unsigned int, JS::Value*)+920>:    mov    (%edi,%eax,4),%ebp
   0x818fafb <array_join(JSContext*, unsigned int, JS::Value*)+923>:    mov    %ebp,(%edx,%eax,4)


This just crashes in a threadsafe optimized 32 bit build. I tried using a debug (or debug+opt) build and it did not reproduce, neither did it reproduce in a non-threadsafe optimized 32 bit build. In all of these cases I got an "InternalError: allocation size overflow" instead of a crash.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

I can't reproduce this locally but I'm pretty sure this should fix it. We were using a RootedString for the separator instead of a Rooted<JSStableString*> so the conservative scanner could destroy it, thereby invalidating our jschar * pointer.

This patch uses a JSStableString and restructures the code a bit so that StringSeparatorOp can use a StableCharPtr instead of jschar *.

decoder: would be great if you could verify this fixes the crash :)

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Created attachment 815364 [details] [diff] [review]
> Patch
> 
> I can't reproduce this locally but I'm pretty sure this should fix it. We
> were using a RootedString for the separator instead of a
> Rooted<JSStableString*> so the conservative scanner could destroy it,
> thereby invalidating our jschar * pointer.

I agree, that's probably what's going on.

> This patch uses a JSStableString and restructures the code a bit so that
> StringSeparatorOp can use a StableCharPtr instead of jschar *.

That's going to force an extra malloc for most string joins. Since this problem is going to go away as soon as we turn on exact rooting, would you be okay with just throwing an Anchor at it?

Comment 6

[Christian Holler (:decoder)]

(In reply to Jan de Mooij [:jandem] from comment #3)
> 
> decoder: would be great if you could verify this fixes the crash :)

Yep, this fixes the issue :)

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Use an Anchor.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 815784 [details] [diff] [review]
Patch v2

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

Thanks! r=me

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40fbd6880056

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40fbd6880056

Test?

Comment 11

[Christian Holler (:decoder)]

I don't think this can be tested reliably at all.

Comment 12

[David Chan [:dchan]]

:jandem

Do you know if this affects b2g18? The code was refactored in bug 918808 from array_join_sub to ArrayJoin

http://mxr.mozilla.org/mozilla-b2g18/source/js/src/jsarray.cpp#1476

Comment 13

[Terrence Cole [:terrence]]

Adding needinfo.

Comment 14

[Jan de Mooij [:jandem]]

(In reply to David Chan [:dchan] from comment #12)
> :jandem
> 
> Do you know if this affects b2g18? The code was refactored in bug 918808
> from array_join_sub to ArrayJoin

No this was a regression from bug 918808 and only affected m-c (Firefox 27).