929280 - Assertion failure: efs->argCount == args.length() - 1, at vm/SelfHosting.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack without symbols

x = ParallelArray()
Array.prototype.shift.call(x.shape)
x.scatter(function() {})

asserts js debug shell on m-c changeset 177bf37a49f5 without any CLI arguments at Assertion failure: efs->argCount == args.length() - 1, at vm/SelfHosting.cpp

Tested with:

https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1382402028/jsshell-mac64.zip

which I presume is a 64-bit debug non-deterministic threadsafe build.

Guessing this is ParallelArray-related, setting needinfo for Shu-yu. This is also flooding jsfunfuzz results and seems to be fairly recent, setting fuzzblocker.

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Shu-yu Guo [:shu]]

I think Felix touched this last, setting needinfo to him. Probably a case where his new assert caught a bug in existing PJS code.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Assert was introduced by Bug 928029.

This is almost certainly an incorrect ThrowError invocation; I will look at it in a bit.

Comment 4

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

(In reply to Felix S. Klock II [:pnkfelix, :fklock] from comment #3)
> Assert was introduced by Bug 928029.
> 
> This is almost certainly an incorrect ThrowError invocation; I will look at
> it in a bit.

Assigning to Felix as per comment 3.

Comment 5

[Christian Holler (:decoder)]

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/651ea35bfb0b
user:        Felix S. Klock II
date:        Mon Oct 21 10:08:01 2013 -0400
summary:     Bug 928029: SelfHosting: check ThrowError argument count (r=till).

This iteration took 423.710 seconds to run.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Christian Holler (:decoder) from comment #5)
> 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/651ea35bfb0b
> user:        Felix S. Klock II
> date:        Mon Oct 21 10:08:01 2013 -0400
> summary:     Bug 928029: SelfHosting: check ThrowError argument count
> (r=till).
> 
> This iteration took 423.710 seconds to run.

The aforementioned changeset is related only in the sense that shu mentioned above: it adds an assert to catch a bug that was previously not being caught (and the failure to catch it yields undefined behavior).

Sorry I've let this sit for so long, I was traveling and then distracted.

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Oddly enough, my own debug shell of mozilla-inbound did not duplicate the problem.  (But I *do* see the assertion fire when I download the copy of jsshell-mac64.zip that gkw linked.)

At this point I am assuming this is due to some build host configuration issue.

Is there an easy way for me to see the configure invocation (or mach invocation?) that was used to build the shell in question?

Comment 8

[Christian Holler (:decoder)]

Since gkw is still on PTO, I can only guess, but did you build a threadsafe shell? What configuration flags, platform and OS did you try?

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

Mac OS X.  64-bit (at least, I think that's the default, since it can create both 32- and 64-bit binaries).  Configure invocations I have tried:

/Users/fklock/Dev/Mozilla/mozilla-inbound/js/src/configure --enable-debug --disable-optimize --with-ccache --enable-threadsafe --with-system-nspr --with-nspr-prefix=/Users/fklock/opt/nspr-dbg --prefix=~/opt/js-dbg-nopt

/Users/fklock/Dev/Mozilla/mozilla-inbound/js/src/configure --enable-debug --enable-optimize --with-ccache --enable-threadsafe --with-system-nspr --with-nspr-prefix=/Users/fklock/opt/nspr-dbg --prefix=~/opt/js-dbg

i.e. I've been using --enable-debug, and I tried toggling between enable/disable optimize.  (The rest of the invocation is boilerplate I've acquired, e.g. I always do --enable-threadsafe since I'm usually doing PJS development when I'm working in the shell.)

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(I am now attempting to build off of mozilla-central at the revision that gkw pointed at, in order to eliminate the possibility that the bug has been somehow fixed on mozilla-inbound in the meantime...  but it would be good to know what command invocation the auto-builder is using.)

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Okay, I can reproduce on mozilla-central.

Odd, I didn't think any of the relevant argument checking code in PJS had changed in mozilla-inbound recently, but maybe I'm wrong.

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

(mystery solved: it didn't reproduce on my copy of mozilla-inbound because I was working with an out-of-date copy.)

The bug is easy to fix, will post patch shortly.

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: fix-throwerror-invocation-arities

Fix the bug.

Before my assert was added from 928029, the test case in question would print the exception:

    RangeError: invalid array length

Which I'm guessing was a bogus message for someone who is familiar with what the above is supposed to mean (versus what it meant in this context).

Then my assert landed, and while attempting to construct an exception like the above, it would hit the assert, and thus you'd get:

    Assertion failure: efs->argCount == args.length() - 1, at [...]

Now with this patch, you get:

    RangeError: invalid ParallelArray.prototype.scatter length argument

Its using the already-overloaded JSMSG_PAR_ARRAY_BAD_ARG to get this effect.

(Also, I'm deliberately attempting to leave it ambiguous as to whether `length` above refers to the length of the receiving parallel-array or the length argument for the target scatter output.)

Fine-tuning all this is probably wasted effort since ParallelArray should go away in the near future (and replaced with Array for the 1-d cases and TypedObjects for the N-d cases).

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

Pushed to inbound:

    https://hg.mozilla.org/integration/mozilla-inbound/rev/75ea7b7bb293

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/75ea7b7bb293

Comment 16

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

Do you mind nominating for aurora landing please?

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

FYI this is the try run from my push to inbound: 

  https://tbpl.mozilla.org/?tree=Try&rev=6251e5b164a4

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 823115 [details] [diff] [review]
patch A v1: fix-throwerror-invocation-arities

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 928029 added a strict check in Debug builds of argument count on ThrowError invocations.  But there was a pre-existing ThrowError invocation that passed an extra (ignored) argument; it causes the check to fire, so we get assertion failures on Debug builds.  Fix here is to switch to using an error code with a matching argument count.

User impact if declined: Fuzz testing on Debug builds for Aurora will be cluttered with the assert check failures noted above.  (gkw has requested this fix go into Aurora.)

Testing completed (on m-c, etc.): JSRefTest debug/opt on moz-inbound

Risk to taking this patch (and alternatives if risky): low risk, IMO.  (There is an alternative of loosening the debug assert to only require that the actual arg count be >= expected arg count.  But I do not think that is worth the effort; better to fix the erroneous ThrowError calls.)

String or IDL/UUID changes made by this patch: none AFAIK.

Comment 19

[Niko Matsakis [:nmatsakis]]

ParallelArray isn't enabled on Aurora builds, so it shouldn't be necessary to land this to aurora I don't think.

Comment 20

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 823115 [details] [diff] [review]
patch A v1: fix-throwerror-invocation-arities

(removing aurora landing request after discussion with nmatsakis since PJS code should be ifdef'ed out on Aurora, and thus this bug and the patch should not have any effect there.)

Comment 21

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

(In reply to Felix S. Klock II [:pnkfelix, :fklock] from comment #20)
> Comment on attachment 823115 [details] [diff] [review]
> patch A v1: fix-throwerror-invocation-arities
> 
> (removing aurora landing request after discussion with nmatsakis since PJS
> code should be ifdef'ed out on Aurora, and thus this bug and the patch
> should not have any effect there.)

Thanks for the clarification!

Comment 22

[Ioana (away)]

Reproducible with the 10/25 Nightly JS shell on Mac OS X 10.9:
Assertion failure: efs->argCount == args.length() - 1, at ../../../js/src/vm/SelfHosting.cpp:87
Segmentation fault: 11

The 02/04 Beta shell and the 02/05 Nightly shell both give this:
ReferenceError: ParallelArray is not defined

No assertion failures here though.