Closed Bug 1221747 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: gkw, Assigned: jonco)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, regression, testcase, Whiteboard: [fuzzblocker][jsbugmon:])

Crash Data

Attachments

(4 files)

// 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)
Flags: needinfo?(jcoppeard)
Crash Signature: [@ IonScriptCounts] [@ js::jit::CodeGenerator::maybeCreateScriptCounts]
Attached file 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)
$ 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.
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Jon, is bug 1212469 a likely regressor?

Nope, it's related to IonScriptCounts.
Assignee: nobody → jcoppeard
No longer blocks: 1212469
Flags: needinfo?(jcoppeard)
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.
Attachment #8683728 - Flags: review?(nicolas.b.pierron)
Attachment #8683728 - Flags: review?(nicolas.b.pierron) → review+
Flags: needinfo?(jcoppeard)
Crash Signature: [@ IonScriptCounts] [@ js::jit::CodeGenerator::maybeCreateScriptCounts] → [@ IonScriptCounts] [@ js::jit::CodeGenerator::maybeCreateScriptCounts]
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
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.
Flags: needinfo?(jcoppeard)
Attachment #8686531 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8686531 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8686531 - Flags: review+
(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.
Crash Signature: [@ IonScriptCounts] [@ js::jit::CodeGenerator::maybeCreateScriptCounts] → [@ IonScriptCounts] [@ js::jit::CodeGenerator::maybeCreateScriptCounts]
Flags: needinfo?(hv1989)
Flags: needinfo?(jcoppeard)
Yes, it looks like the same issue.
Flags: needinfo?(jcoppeard)
Duplicate of this bug: 1205130
> Yes, it looks like the same issue.

Thanks! (and duped)
Flags: needinfo?(hv1989)
(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!
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].
OS: Linux → All
Whiteboard: [jsbugmon:] → [fuzzblocker][jsbugmon:]
Attached patch fix-js-vsnprintfSplinter Review
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.
Attachment #8687998 - Flags: review?(nicolas.b.pierron)
Attachment #8687998 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/408e4d2185f6
https://hg.mozilla.org/mozilla-central/rev/fb77eaf689fb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.