1221747 - Crash [@ IonScriptCounts] or [@ js::jit::CodeGenerator::maybeCreateScriptCounts]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

// Adapted from randomly chosen test: js/src/jit-test/tests/gc/bug-1214006.js
function f() {
    eval("(function() {})()");
}
oomTest(f);

crashes js debug shell on m-c changeset 6077f51254c6 with --fuzzing-safe --no-threads --ion-eager -D at IonScriptCounts with js::jit::CodeGenerator::maybeCreateScriptCounts on the stack.

Configure options:

AR=ar sh /home/ubuntu/trees/mozilla-central/js/src/configure --enable-debug --disable-threadsafe --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic -R /home/ubuntu/trees/mozilla-central" -r 6077f51254c6

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9c365490d4ce
user:        Jon Coppeard
date:        Tue Oct 13 13:37:07 2015 +0100
summary:     Bug 1212469 - Make oomTest() into a shell function r=nbp

Jon, is bug 1212469 a likely regressor? (The issue seems to go away without oomTest)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(gdb) bt 5
#0  ~IonScriptCounts (this=<optimized out>, __in_chrg=<optimized out>) at /home/ubuntu/trees/mozilla-central/js/src/jit/IonCode.h:704
#1  js_delete<js::jit::IonScriptCounts> (p=0x7ffff5599940) at ../../dist/include/js/Utility.h:370
#2  0x00000000005c7428 in js::jit::CodeGenerator::maybeCreateScriptCounts (this=this@entry=0x7ffff55b2000) at /home/ubuntu/trees/mozilla-central/js/src/jit/CodeGenerator.cpp:3831
#3  0x00000000005fb08b in js::jit::CodeGenerator::generateBody (this=this@entry=0x7ffff55b2000) at /home/ubuntu/trees/mozilla-central/js/src/jit/CodeGenerator.cpp:4085
#4  0x00000000005fbb5a in js::jit::CodeGenerator::generate (this=this@entry=0x7ffff55b2000) at /home/ubuntu/trees/mozilla-central/js/src/jit/CodeGenerator.cpp:7887
(More stack frames follow...)
(gdb)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

$ cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=15.04
DISTRIB_CODENAME=vivid
DISTRIB_DESCRIPTION="Ubuntu 15.04"

$ gcc --version
gcc-4.9.real (Ubuntu 4.9.2-10ubuntu13) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Jon, is bug 1212469 a likely regressor?

Nope, it's related to IonScriptCounts.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1221747-ion-script-counts

Change IonScriptCounts::init() to only set numBlocks_ if initialisation of blocks_ succeeds.

Give Sprinter the option to not report OOM to the context and use this to ignore OOM when printing in ScriptBlockCountState.

I also renamed Sprinter::reportedOOM_ to hadOOM_ since that better reflects its use.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23f47084749f

Comment 6

[Phil Ringnalda (:philor)]

Backed out in  - Windows shell build assertions like https://treeherder.mozilla.org/logviewer.html#?job_id=16914571&repo=mozilla-inbound, and jittest hangs like https://treeherder.mozilla.org/logviewer.html#?job_id=16922728&repo=mozilla-inbound

Comment 7

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05da298a22b

Comment 8

[Fuzzing Team]

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

Comment 9

[Jon Coppeard (:jonco)]

Attachment: bug1221747-vsnprintf-fix

It seems that on Windows vsnprintf() behaves differently on failure, returning -1 and leaving the buffer not null terminated.  This causes the final assert to fail in Sprinter::checkInvariants().  I've added a fix here.  Let me know if you think it's worth replacing all our use of this function with a shim that has consistent behaviour - it only seems to be used for debugging and spew.

Comment 10

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8686531 [details] [diff] [review]
bug1221747-vsnprintf-fix

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

::: js/src/vm/Printer.cpp
@@ +227,5 @@
>              return i;
>          }
> +#ifdef XP_WIN
> +        // Windows doesn't necessarily null terminate the buffer on failure.
> +        base[size - 1] = 0;

Remove this special case, and use JS_vsnprintf instead of vsnprintf.

Comment 11

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8686531 [details] [diff] [review]
bug1221747-vsnprintf-fix

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

r=me with what is suggested in comment 10 applied.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Jon Coppeard (:jonco) from comment #9)
> This causes the final assert to fail in Sprinter::checkInvariants().

Is this related to bug 1205130? Hannes couldn't reproduce that Windows-only issue, and the assertion / stack looks related.

Comment 13

[Jon Coppeard (:jonco)]

Yes, it looks like the same issue.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Yes, it looks like the same issue.

Thanks! (and duped)

Comment 16

[Jon Coppeard (:jonco)]

(In reply to Nicolas B. Pierron [:nbp] [off until 2015-11-18] from comment #10)
> Remove this special case, and use JS_vsnprintf instead of vsnprintf.

This is proving surprisingly difficult.  JS_vsnprintf doesn't correctly indicate failure, and even with that fixed it sometimes seems to print the wrong thing!

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is also appearing on Windows, and is also the cause of some hard-to-reproduce issues that seem to reduce to this bug, so setting [fuzzblocker].

Comment 18

[Jon Coppeard (:jonco)]

Attachment: fix-js-vsnprintf

Patch to fix JS_vsnprintf() to return a value that indicates failure if we reached the buffer limit.  It turns out that JS_vsnprintf() does print correctly, it's just called in a non-threadsafe way from the backtracking register allocator with a single buffer used from multiple threads.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/408e4d2185f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb77eaf689fb

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/408e4d2185f6
https://hg.mozilla.org/mozilla-central/rev/fb77eaf689fb