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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: 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(),

Comment 1

[Christian Holler (:decoder)]

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)

Comment 2

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

Shu-yu, is it possible for you to take a look at this? (comment 1 points to a changeset by you in bug 778559)

Comment 3

[Shu-yu Guo [:shu]]

Attachment: fix

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.

Comment 4

[Jason Orendorff [: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.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: fix

Applied jorendorff's comments

Comment 6

[Jason Orendorff [:jorendorff]]

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. :-\

Comment 7

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

jorendorff, do you mean r+ instead of r? here?

Comment 8

[Shu-yu Guo [:shu]]

(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 9

[Shu-yu Guo [:shu]]

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

https://tbpl.mozilla.org/?tree=Try&rev=168130ece4ba

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 662723 [details] [diff] [review]
fix

Yes, that's what I meant. Sorry.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Shu-yu Guo [:shu]]

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.

Comment 13

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

Shu-yu landed this:

https://hg.mozilla.org/releases/mozilla-aurora/rev/d45511cf87b2

Comment 14

[Simona Badau, Desktop QA]

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

Comment 15

[Simona Badau, Desktop QA]

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

Comment 16

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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

Comment 17

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old