Closed Bug 791445 Opened 7 years ago Closed 7 years ago

"Assertion failure: !cx->isExceptionPending(),"

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox15 --- unaffected
firefox16 --- unaffected
firefox17 --- verified
firefox18 --- verified
firefox-esr10 --- unaffected

People

(Reporter: gkw, Assigned: shu)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 1 obsolete file)

Attached file stack
ParallelArray(0, Proxy.createFunction(function(){}, function(){}))

asserts js debug shell on m-c changeset 9fff2012b66c without any CLI arguments at Assertion failure: !cx->isExceptionPending(),
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:   102665:ea2ad8970f3e
user:        Shu-yu Guo
date:        Fri Aug 17 10:38:59 2012 -0700
summary:     Bug 778559 - Implement ParallelArray API with sequential execution (r=dmandelin)
Shu-yu, is it possible for you to take a look at this? (comment 1 points to a changeset by you in bug 778559)
Assignee: general → shu
Attached patch fix (obsolete) — Splinter Review
This is a debug-only bug. When DEBUG is true, all the ParallelArray methods can take an extra options object to assert things like a certain mode of execution should fail/succeed. I had not realized that getting properties on that object might throw.
Attachment #661542 - Flags: review?(dmandelin)
Attachment #661542 - Flags: review?(dmandelin) → review?(jorendorff)
> bool
> ParallelArrayObject::DebugOptions::init(JSContext *cx, const Value &v)
> {
>     if (!v.isObject())
>         return false;

By convention, functions like this should return false iff an error occurred on cx and hasn't been handled yet. So what you'd do here is:

    RootedObject obj(cx, NonNullObject(cx, v));
    if (!obj)
        return false;

NonNullObject throws an exception for you if v isn't an object.
Attachment #661542 - Flags: review?(jorendorff)
Attached patch fixSplinter Review
Applied jorendorff's comments
Attachment #661542 - Attachment is obsolete: true
Comment on attachment 662723 [details] [diff] [review]
fix

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

Looks good. r=me with the few trouble spots fixed.

::: js/src/builtin/ParallelArray.cpp
@@ +702,5 @@
>          return false;
>  
>      propStr = ToString(cx, propv);
>      if (!JS_StringEqualsAscii(cx, propStr, "par", &match))
> +        return ReportBadArg(cx);

If JS_StringEqualsAscii fails, an error has already been reported, so just return false.

@@ +707,5 @@
>      if (match) {
>          mode = ∥
>      } else {
> +        if (!JS_StringEqualsAscii(cx, propStr, "seq", &match) || !match)
> +            return ReportBadArg(cx);

As above. This means you need four lines, unless you're going to do something clever:

    if (!JS_StringEqualsAscii(cx, propStr, "seq", &match))
        return false;
    if (!match)
        return ReportBadArg(cx);

The same in the other places where it's called.

@@ +715,5 @@
>      id = AtomToId(Atomize(cx, "expect", strlen("expect")));
>      if (!JSObject::getGeneric(cx, obj, obj, id, &propv))
>          return false;
>  
>      propStr = ToString(cx, propv);

Need to check after this:  if (!propStr) return false;

@@ +722,3 @@
>      if (match) {
>          expect = ExecutionFailed;
>      } else {

Here you could write:

    bool ok;
    if ((ok = JS_StringEqualsAscii(..."fail"...)) && match)
        expect = ExecutionFailed;
    else if (ok && (ok = JS_StringEqualsAscii(..."bail"...)) && match)
        expect = ExecutionCompiled;
    else if (ok && (ok = JS_StringEqualsAscii(..."success"...) && match)
        expect = ExecutionSucceeded;
    else if (ok)
        return ReportBadArg(cx);
    else
        return false;

Kind of gross, but that's checking return values for you. :-\
Attachment #662723 - Flags: review?
Keywords: regression
Blocks: 778559
jorendorff, do you mean r+ instead of r? here?
(In reply to Jason Orendorff [:jorendorff] from comment #6)
>     bool ok;
>     if ((ok = JS_StringEqualsAscii(..."fail"...)) && match)
>         expect = ExecutionFailed;
>     else if (ok && (ok = JS_StringEqualsAscii(..."bail"...)) && match)
>         expect = ExecutionCompiled;
>     else if (ok && (ok = JS_StringEqualsAscii(..."success"...) && match)
>         expect = ExecutionSucceeded;
>     else if (ok)
>         return ReportBadArg(cx);
>     else
>         return false;
> 
> Kind of gross, but that's checking return values for you. :-\

Less gross than what I was doing before; will switch to this style.
Comment on attachment 662723 [details] [diff] [review]
fix

Yes, that's what I meant. Sorry.
Attachment #662723 - Flags: review? → review+
https://hg.mozilla.org/mozilla-central/rev/e7164530358f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 662723 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
   Bug #778559
User impact if declined:
  This patch will help with fuzzing on Aurora.  The only impact of declining
  this would be less security testing.
Testing completed (on m-c, etc.): 
  on m-c.
Risk to taking this patch (and alternatives if risky): 
  None.
String or UUID changes made by this patch: 
  None.
Attachment #662723 - Flags: approval-mozilla-aurora?
Attachment #662723 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified the fix by running the test from bug description on Spidermonkey built from the latest mozilla-beta (ae08e43155c3) source: no assertion occurs.
OS: Mac OS X 10.7.5
Verified the fix by running the test from the bug description on Spidermonkey built from the latest mozilla-beta (cf5d5b6ddee8) source: no assertion occurs.
OS: Mac OS X 10.7.5
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.