Closed Bug 1435321 Opened 2 years ago Closed 2 years ago

Assertion failure: factor >= minRatio, at js/src/jsgc.cpp:1855 with gcparam

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update,bisect])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 841512e696b9 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --disable-oom-functions --disable-oom-functions --baseline-eager):

a2 = gcparam('highFrequencyHeapGrowthMax', 120);
var actual = '';
for (var i = 0; i < 5000000; ++i)
  actual += "foobar";


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00000000009db186 in js::gc::ZoneHeapThreshold::computeZoneHeapGrowthFactorForHeapSize (lastBytes=<optimized out>, tunables=..., state=...) at js/src/jsgc.cpp:1855
#0  0x00000000009db186 in js::gc::ZoneHeapThreshold::computeZoneHeapGrowthFactorForHeapSize (lastBytes=<optimized out>, tunables=..., state=...) at js/src/jsgc.cpp:1855
#1  0x00000000009db1ee in js::gc::ZoneHeapThreshold::updateAfterGC (this=0x7ffff55eba58, lastBytes=lastBytes@entry=111501312, gckind=gckind@entry=GC_NORMAL, tunables=..., state=..., lock=...) at js/src/jsgc.cpp:1878
#2  0x00000000009f05b1 in js::gc::GCRuntime::endSweepingSweepGroup (this=0x7ffff5f1a780, fop=<optimized out>, budget=...) at js/src/jsgc.cpp:5651
#3  0x0000000000a34c60 in sweepaction::SweepActionSequence<js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7ffff5f172e0, args#0=0x7ffff5f1a780, args#1=0x7fffffffc1a0, args#2=...) at js/src/jsgc.cpp:6199
#4  0x0000000000a2396d in sweepaction::SweepActionRepeatFor<js::gc::SweepGroupsIter, JSRuntime*, js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run (this=0x7ffff5f18fd0, args#0=0x7ffff5f1a780, args#1=0x7fffffffc1a0, args#2=...) at js/src/jsgc.cpp:6260
#5  0x0000000000a0d59e in js::gc::GCRuntime::performSweepActions (this=this@entry=0x7ffff5f1a780, budget=..., lock=...) at js/src/jsgc.cpp:6412
#6  0x0000000000a1123e in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff5f1a780, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER, lock=...) at js/src/jsgc.cpp:6974
#7  0x0000000000a12573 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f1a780, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER) at js/src/jsgc.cpp:7311
#8  0x0000000000a12c5d in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f1a780, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::ALLOC_TRIGGER) at js/src/jsgc.cpp:7454
#9  0x0000000000a13212 in js::gc::GCRuntime::gcSlice (this=0x7ffff5f1a780, reason=JS::gcreason::ALLOC_TRIGGER, millis=0) at js/src/jsgc.cpp:7543
#10 0x0000000000a132d7 in js::gc::GCRuntime::gcIfRequested (this=0x7ffff5f1a780) at js/src/jsgc.cpp:7721
#11 0x0000000000bf5268 in InvokeInterruptCallback (cx=0x7ffff5f16000) at js/src/vm/Runtime.cpp:527
#12 0x00001dd2b9756c19 in ?? ()
[...]
#15 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff5f1b4b8	140737319646392
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbf80	140737488338816
rsp	0x7fffffffbf40	140737488338752
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x6a56000	111501312
r13	0x7ffff5f1b598	140737319646616
r14	0x0	0
r15	0x7fffffffc080	140737488339072
rip	0x9db186 <js::gc::ZoneHeapThreshold::computeZoneHeapGrowthFactorForHeapSize(unsigned long, js::gc::GCSchedulingTunables const&, js::gc::GCSchedulingState const&)+486>
=> 0x9db186 <js::gc::ZoneHeapThreshold::computeZoneHeapGrowthFactorForHeapSize(unsigned long, js::gc::GCSchedulingTunables const&, js::gc::GCSchedulingState const&)+486>:	movl   $0x0,0x0
   0x9db191 <js::gc::ZoneHeapThreshold::computeZoneHeapGrowthFactorForHeapSize(unsigned long, js::gc::GCSchedulingTunables const&, js::gc::GCSchedulingState const&)+497>:	ud2
This stack looks weird to me even if it is consistent with the test case content, feel free to redirect to the proper person when we have the bisection result.
Flags: needinfo?(jcoppeard)
Priority: -- → P1
The problem here is that we allow the 'high frequency heap growth min' parameter to be set higher than the 'high frequency heap growth max' parameter.

The patch factors out setting these paraemters into separate methods as is done for some of the other parameters and makes sure that we update the corresponding parameter to preserve the invaraint max >= min when necessary.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8948953 - Flags: review?(pbone)
Comment on attachment 8948953 [details] [diff] [review]
bug1435321-factor-assert

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

This is good except for one potential change below.  r+ and you can choose if highFrequencyLowLimit needs changing.

::: js/src/jit-test/tests/gc/bug-1435321.js
@@ +26,5 @@
> +assertEq(gcparam('highFrequencyHighLimit'), 100);
> +
> +gcparam('highFrequencyLowLimit', 300);
> +assertEq(gcparam('highFrequencyLowLimit'), 300);
> +assertEq(gcparam('highFrequencyHighLimit'), 300);

When setting the high limit the low limit is set to 1MiB below it. Why doesn't that happen with when setting the high limit? or is it some rounding-off?  If you change the behavour could you make it a seperate patch add add these tests (parameters ending in "Limit") in that patch.

Thanks.

::: js/src/jsgc.cpp
@@ +1406,5 @@
> +{
> +    highFrequencyHeapGrowthMin_ = value;
> +    if (highFrequencyHeapGrowthMin_ > highFrequencyHeapGrowthMax_)
> +        highFrequencyHeapGrowthMax_ = highFrequencyHeapGrowthMin_;
> +    MOZ_ASSERT(highFrequencyHeapGrowthMin_ / 0.85 > 1.0);

I can see that's equivilent to x > 0.85, I havn't seen thsi pattern before. Is it some numerical method? For my own curiosity, why is it important and why doesn't the compiler do it?

Why not also use it to compare with highFrequencyHeapTrowthMax_?
Attachment #8948953 - Flags: review?(pbone) → review+
(In reply to Paul Bone [:pbone] from comment #3)
> When setting the high limit the low limit is set to 1MiB below it. Why
> doesn't that happen with when setting the high limit? or is it some
> rounding-off?  

Yes, it's rounding.  Here the low limit is set to one *byte* below the high limit internally, but the interface scales these values to MiB and rounds down.  So 1 byte under appears as 1MiB under, but one byte over appears as the same value.

> I can see that's equivilent to x > 0.85, I havn't seen thsi pattern before.
> Is it some numerical method? For my own curiosity, why is it important and
> why doesn't the compiler do it?

That is a really good question.  I don't think it's a numerical thing.  Oh, it's a bug!  I filed bug 1436697.  Thanks for raising this.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce744f3cd76
Preserve invariants when setting high frequency heap growth parameters r=pbone
https://hg.mozilla.org/mozilla-central/rev/0ce744f3cd76
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.