Closed Bug 973571 Opened 6 years ago Closed 6 years ago

Assertion failure: isEmpty(), at jsgc.h:993 with gcslice and gcparam

Categories

(Core :: JavaScript: GC, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: decoder, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files, 1 obsolete file)

The following testcase asserts on mozilla-central revision 2bddbd180d2d (run with --fuzzing-safe):


gcslice(1);
gcparam('markStackLimit', 5);
Probably a shell-only problem with these two debug functions.
Whiteboard: [jsbugmon:update,bisect]
gcslice and gcparam both exist in the browser, as do the heuristics they override. Why would this bug (or testcase) be shell-only?
(In reply to Jesse Ruderman from comment #3)
> gcslice and gcparam both exist in the browser, as do the heuristics they
> override. Why would this bug (or testcase) be shell-only?

I assumed that because the failure happens inside one of the testing functions. It's not that two parameters are tweaked and then something happens, but the assert happens while the second debug function (gcparam) is called here:


Program received signal SIGSEGV, Segmentation fault.
0x00000000007e3dd4 in setMaxCapacity (this=0x17e3340, maxCapacity=5) at js/src/jsgc.h:993
993             JS_ASSERT(isEmpty());
(gdb) bt
#0  0x00000000007e3dd4 in setMaxCapacity (this=0x17e3340, maxCapacity=5) at js/src/jsgc.h:993
#1  setMaxCapacity (maxCap=5, this=0x17e3308) at js/src/jsgc.h:1122
#2  js::SetMarkStackLimit (rt=0x17e2ed0, limit=5) at js/src/jsgc.cpp:2097
#3  0x000000000047af7a in GCParameter (cx=0x1808ee0, argc=<optimized out>, vp=0x18a75b0) at js/src/builtin/TestingFunctions.cpp:334
#4  0x00000000008feeb1 in js::CallJSNative (cx=0x1808ee0, native=0x47ad70 <GCParameter(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:230


That's why I assume this is debug-only (though, if both functions are exposed in the browser, it can happen in debug builds there too).
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/f1e8ada1ddf7
user:        Ben Turner
date:        Thu Jul 25 15:03:27 2013 -0400
summary:     Bug 894993 - 'SessionWorker takes lots of memory'. r=billm+khuey.

This iteration took 333.775 seconds to run.
Needinfo from Ben Turner based on comment 5 :) Ben, if you're not the right person to fix this, can you forward this to billm? Thanks!
Flags: needinfo?(bent.mozilla)
Yeah, I'm definitely not the right person to deal with this.
Flags: needinfo?(bent.mozilla)
Maybe Terrence has some idea.
Component: JavaScript Engine → JavaScript: GC
Flags: needinfo?(terrence)
I think njn looked at this code most recently.
Flags: needinfo?(n.nethercote)
Updating the mark stack size is only allowed when there isn't a GC actively using the mark stack. The API doesn't really promise any safety for misuse, so I'm not sure that this is technically a bug. That said, there's no good reason to leave a loaded footgun in active range of the fuzzing machinery when there's no reason it couldn't just fail instead.
Flags: needinfo?(terrence)
I don't have anything insightful to add to Terrence's diagnosis.

Terrence, I think you're suggesting changing this assertion in SetMarkStackLimit():

    JS_ASSERT(!rt->isHeapBusy());

to this:

    if (rt->isHeapBusy())
        return;

Is that right? SetMarkStackLimit() and JS_SetGCParameter() currently have no notion of failure.
Flags: needinfo?(n.nethercote)
I was actually thinking the check should be done in gcparam in TestingFunctions, so we can continue guarding C++ api usage -- I doubt we're going to be fuzzing /that/ any time soon. However -- and this may be a silly question -- why do we have this in the API at all?

The issue here is that if we cannot malloc during GC we have to oom, so platforms with lots of spare memory can set the reserved size high to decrease the likelyhood of OOM here. Is gecko using this? No. Do we want to do this and throw our startup memory numbers under the bus? Not really. I'd vote for just removing this interface.

Thoughts?
Attachment #8377097 - Attachment is obsolete: true
> why do we have this in the API at all? 

Setting gcparam('markStackLimit') to a very *small* value helps fuzzers find bugs in the stackless fallback mode (e.g. bug 939499 and bug 939476). Without this API, fuzzers would only be able to test it by creating very long chains of objects, which is impractical.
(In reply to Jesse Ruderman from comment #14)
> > why do we have this in the API at all? 
> 
> Setting gcparam('markStackLimit') to a very *small* value helps fuzzers find
> bugs in the stackless fallback mode (e.g. bug 939499 and bug 939476).
> Without this API, fuzzers would only be able to test it by creating very
> long chains of objects, which is impractical.

Thanks, Jesse, that's a great answer! Here is a patch that raises an exception on requests to change the mark stack size while an incremental GC is in progress. This matches other "misuse" cases which raise an Error, so I think this should be okay with the fuzzers.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8388125 - Flags: review?(wmccloskey)
Comment on attachment 8388125 [details] [diff] [review]
gcparam_while_incremental-v0.diff

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

::: js/src/builtin/TestingFunctions.cpp
@@ +319,5 @@
>                             "with non-zero value");
>          return false;
>      }
>  
> +    if (param == JSGC_MARK_STACK_LIMIT && cx->runtime()->gcIncrementalState != gc::NO_INCREMENTAL) {

We also use the mark stack during verification, so please use IsIncrementalGCInProgress instead.
Attachment #8388125 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/b91e62bdacba
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.