1231163 - Assertion failure: script()->compartment()->randomNumberGenerator.isSome() (MRandom JIT code depends on RNG being initialized), at js/src/jit/MCallOptimize.cpp:1364

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision cc9c6cd756cb (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

function f() {
    Math.random;
    return function() {
        +Math.random()
    }
}
f()()


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000700f08 in js::jit::IonBuilder::inlineMathRandom (this=0x7ffff69a01c0, callInfo=...) at js/src/jit/MCallOptimize.cpp:1363
1363	    MOZ_ASSERT(script()->compartment()->randomNumberGenerator.isSome(),
#0  0x0000000000700f08 in js::jit::IonBuilder::inlineMathRandom (this=0x7ffff69a01c0, callInfo=...) at js/src/jit/MCallOptimize.cpp:1363
#1  0x0000000000699305 in js::jit::IonBuilder::inlineSingleCall (this=0x7ffff69a01c0, callInfo=..., targetArg=<optimized out>) at js/src/jit/IonBuilder.cpp:5535
#2  0x0000000000692c2f in js::jit::IonBuilder::inlineCallsite (this=0x7ffff69a01c0, targets=..., callInfo=...) at js/src/jit/IonBuilder.cpp:5599
#3  0x00000000006930e5 in js::jit::IonBuilder::jsop_call (this=this@entry=0x7ffff69a01c0, argc=0, constructing=<optimized out>) at js/src/jit/IonBuilder.cpp:6532
#4  0x0000000000693ca4 in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff69a01c0, op=op@entry=JSOP_CALL) at js/src/jit/IonBuilder.cpp:1885
#5  0x00000000006954aa in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff69a01c0) at js/src/jit/IonBuilder.cpp:1520
#6  0x00000000006961ae in js::jit::IonBuilder::build (this=0x7ffff69a01c0) at js/src/jit/IonBuilder.cpp:916
#7  0x000000000069f386 in js::jit::IonCompile (cx=cx@entry=0x7ffff6918c00, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=js::jit::Optimization_Normal) at js/src/jit/Ion.cpp:2193
#8  0x000000000069ff51 in js::jit::Compile (cx=cx@entry=0x7ffff6918c00, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2431
#9  0x00000000006a03ce in js::jit::CanEnter (cx=cx@entry=0x7ffff6918c00, state=...) at js/src/jit/Ion.cpp:2593
#10 0x0000000000a95c49 in js::RunScript (cx=cx@entry=0x7ffff6918c00, state=...) at js/src/vm/Interpreter.cpp:367
#11 0x0000000000a95f46 in js::Invoke (cx=cx@entry=0x7ffff6918c00, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:462
#12 0x0000000000a96ad9 in js::Invoke (cx=cx@entry=0x7ffff6918c00, thisv=..., fval=..., argc=argc@entry=0, argv=argv@entry=0x7fffffffd058, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:496
#13 0x00000000005d5f1e in js::jit::DoCallFallback (cx=0x7ffff6918c00, frame=0x7fffffffd088, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffd048, res=...) at js/src/jit/BaselineIC.cpp:6149
#14 0x00007ffff7fef4ef in ?? ()
#15 0xe900000a90898b48 in ?? ()
#16 0x00007fffffffd000 in ?? ()
#17 0xfff9000000000000 in ?? ()
#18 0x0000000001b68700 in js::jit::DoSpreadCallFallbackInfo ()
#19 0x00007ffff7e5b640 in ?? ()
#20 0x00007ffff7ff3663 in ?? ()
#21 0x0000000000000802 in ?? ()
#22 0x00007fffffffd088 in ?? ()
#23 0x00007ffff540ce58 in ?? ()
#24 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff69a01c0	140737330676160
rcx	0x7ffff6c294e0	140737333335264
rdx	0x0	0
rsi	0x7ffff6ef8960	140737336281440
rdi	0x7ffff6ef7640	140737336276544
rbp	0x7fffffffc170	140737488339312
rsp	0x7fffffffc150	140737488339280
r8	0x7ffff6ef8960	140737336281440
r9	0x7ffff7fd3740	140737353955136
r10	0x58	88
r11	0x7ffff6ba0be0	140737332775904
r12	0x7fffffffc2f0	140737488339696
r13	0x7fffffffc2f0	140737488339696
r14	0x0	0
r15	0x0	0
rip	0x700f08 <js::jit::IonBuilder::inlineMathRandom(js::jit::CallInfo&)+344>
=> 0x700f08 <js::jit::IonBuilder::inlineMathRandom(js::jit::CallInfo&)+344>:	movl   $0x0,0x0
   0x700f13 <js::jit::IonBuilder::inlineMathRandom(js::jit::CallInfo&)+355>:	ud2

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d0948f31ce27
user:        Jan de Mooij
date:        Wed Dec 02 13:56:00 2015 +0100
summary:     Bug 322529 part 1 - Use XorShift128PlusRNG for Math.random(). r=jwalden

changeset:   https://hg.mozilla.org/mozilla-central/rev/182369db983e
user:        Jan de Mooij
date:        Wed Dec 02 13:56:00 2015 +0100
summary:     Bug 322529 part 2 - Add xor64 and add64 to macro-assemblers. r=arai

changeset:   https://hg.mozilla.org/mozilla-central/rev/c511942454b6
user:        Jan de Mooij
date:        Wed Dec 02 13:56:00 2015 +0100
summary:     Bug 322529 part 3 - Fix LRandom JIT code to use the new algorithm. r=arai,jwalden

changeset:   https://hg.mozilla.org/mozilla-central/rev/a0d56a24423c
user:        Jan de Mooij
date:        Wed Dec 02 13:56:01 2015 +0100
summary:     Bug 322529 part 4 - Fix setRNGState shell function and math-random.js jit-test. r=arai

This iteration took 216.367 seconds to run.

Comment 2

[Victor Carlquist]

Attachment: Patch.

I took the liberty to solve this.
If this patch is wrong, feel free to cancel it, because,
as it is a critical bug and I am a beginner yet, I don't want to bother you (:

Comment 3

[Victor Carlquist]

Attachment: bug1231163.patch

I took the liberty to solve this.
If this patch is wrong, feel free to cancel it, because,
as it is a critical bug and I am a beginner yet, I don't want to bother you (:

Comment 4

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

Bug 322529 is likely the regressor, probably, based on the regression range in comment 1.

Comment 5

[Tooru Fujisawa [:arai]]

I believe this happens only in shell with --ion-eager flag (or equivalent configuration), otherwise RNG should be initialized while running same script in the interpreter.
IMO, there could be some sanity check for normal configuration.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8696839 [details] [diff] [review]
bug1231163.patch

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

Looks good as a fix, just nitpicking a little.  Mind posting an updated patch along these lines?

::: js/src/jit-test/tests/ion/bug1231163.js
@@ +3,5 @@
> +    return function() {
> +        +Math.random();
> +    }
> +}
> +f()()

The test file should be named more descriptively -- inline-Math-random-before-called.js, say.

As for the test contents, I'd like to see a test that describes what it's testing better, and I'd like the test to be clear what it's testing.  This alteration of the test, I think, covers this:

// |jit-test| ion-eager

function ionCompiledEagerly() {
  Math.random; // establish Math.random's identity for inlining
  return function() {
    return +Math.random(); // call will be inlined
  };
}
var alreadyIonCompiled = ionCompiledEagerly();
assertEq(alreadyIonCompiled() < 1, true);

::: js/src/jit/MCallOptimize.cpp
@@ +1359,5 @@
>  
>      if (getInlineReturnType() != MIRType_Double)
>          return InliningStatus_NotInlined;
>  
> +    // MRandom JIT code depends on RNG being initialized.

It'd be nice to additionally say why this isn't guaranteed already, versus just saying why we're doing this.  How does this comment sound to you?

    // MRandom JIT code directly accesses the RNG  It's (barely) possible to
    // inline Math.random without it having been called yet, so ensure RNG
    // state that isn't guaranteed to be initialized already.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> I believe this happens only in shell with --ion-eager flag (or equivalent
> configuration), otherwise RNG should be initialized while running same
> script in the interpreter.

The thing is, I thought this assertion would hold, because we check the observed type is MIRType_Double. To know that, we must have executed the Math.random() call.

Here, though, we're doing +Math.random() - I think somewhere the JSOP_POS (for unary +) is adding MIRType_Double to the result type set if it's empty. We should probably stop doing that at some point, but this fix seems fine for now.

Comment 8

[Victor Carlquist]

Attachment: Patch updated.

Thank you for the review.

I am posting a patch with the modifications suggested.

Comment 9

[Victor Carlquist]

Try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09228d815284

Comment 10

[Victor Carlquist]

(In reply to Victor Carlquist from comment #9)
> Try server:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=09228d815284

Try is green, but I can't neither land this patch nor adding the 'checkin-need' on keywords, because I don't have permission.

Could someone land this patch to me, please?

Comment 11

[Jeff Walden [:Waldo]]

Was going to do that right now.  :-)  I just updated my system, tho, so I have some futzing to do to get back to a point where I can land stuff.  Today, tho.

Comment 12

[Victor Carlquist]

Ok! Thanks :)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee132b90ad4b

Comment 14

[Jeff Walden [:Waldo]]

I tweaked the commit message slightly on landing, to this:

changeset:   276216:ee132b90ad4b
tag:         tip
user:        Victor Carlquist <[e-mail]>
date:        Wed Dec 09 22:14:53 2015 -0200
summary:     Bug 1231163 - Don't assume the RNG's been initialized by a prior call to Math.random, when a call to Math.random is being inlined.  (A method can be inlined once its identity has been guarded against, but mere identity can be established without the method having been called.)  r=jwalden

Generally it's good for commit messages to explain approximately what the problem was that was fixed.  "Fixed RNG initialization" says what you did, and it implies it previously was broken, without explaining how it was fixed or why it needed fixing.  This new message says *what* we did wrong (assume RNG initialization), when the problem occurs (the bad assumption is made during inlining), and why (the assumption of having-been-called is not valid during inlining).  Make sense?  :-)

(And on a more frivolous note, I super-mildly prefer "r=jwalden" because "r=Waldo" would have pulsebot spuriously ping me on IRC.  But this is a negligible thing I wouldn't complain about, and I only changed it because I was touching the commit message already.  :-) )

Thanks for the patch!

Comment 15

[Victor Carlquist]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)

Awesome!

> Generally it's good for commit messages to explain approximately what the
> problem was that was fixed.  "Fixed RNG initialization" says what you did,
> and it implies it previously was broken, without explaining how it was fixed
> or why it needed fixing.  This new message says *what* we did wrong (assume
> RNG initialization), when the problem occurs (the bad assumption is made
> during inlining), and why (the assumption of having-been-called is not valid
> during inlining).  Make sense?  :-)


I got it, thanks for the nice explanation and the review.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ee132b90ad4b