895792 - Assertion failure: ~x > x (can't round up -- will overflow!), at dist/include/mozilla/MathAlgorithms.h

Status: VERIFIED FIXED

(Core :: MFBT, defect)

Description

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

Attachment: stack

Object.defineProperty(this, "p0", {
    get: function() {
        return p2.partition(1);
    }
});
p2 = new ParallelArray([13266], function(x0) {
    return (gcPreserveCode());
});
p1 = p0.filter(function(e, i, s) {
    return i % 2;
});
uneval(p1);

asserts js debug shell on m-c changeset 0d0263a58f06 without any CLI arguments at Assertion failure: ~x > x (can't round up -- will overflow!), at dist/include/mozilla/MathAlgorithms.h

Setting s-s because this seems to involve gc via gcPreserveCode.

This reproduced with a 32-bit threadsafe shell on Linux (I think both deterministic / non-deterministic), but possibly not 64-bit.

Will be running bisection - assuming ParallelArrays are involved for now.

Comment 1

[Shu-yu Guo [:shu]]

Like the other GC fuzz test, I can't reproduce this. 32bit or 64bit, --enable-debug --enable-optimize, --enable-threadsafe. :/

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/021fd4e03439
user:        Jeff Walden
date:        Wed Jul 03 15:46:51 2013 -0700
summary:     Bug 891177 - Move leading/trailing-zero-bit counting functions, ceiling/floor log2 functions, and round-up-pow2 functions into MathAlgorithms.h.  r=terrence

Waldo, do you think you could take a look and see if bug 891177 is related?

Comment 3

[Jeff Walden [:Waldo]]

The failing assertion is a new one, added in that bug, to track a requirement that wasn't tested before.  The problem is that the Vector code implicated by that stack isn't using RoundUpPow2 correctly.  I'm pretty sure some sort of 32-bit Vector unit test could reproduce the assertion pretty easily.  I'll try to take a look into this soon-ish, but no guarantees, as this is a bit off my current path now.  I don't know whether a failure there has any security implications.  It might, or it might not -- I'd have to investigate the Vector code a bit more before I could tell.

Comment 4

[Christian Holler (:decoder)]

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

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Fix RoundUpPow2's required precondition to not be wrong

Comment 6

[Jeff Walden [:Waldo]]

Not security-sensitive, just a mistaken assertion written by blindly transcribing an incorrect precondition given in a nearby comment -- the actual code works correctly if you fall through the assertion.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 781910 [details] [diff] [review]
Fix RoundUpPow2's required precondition to not be wrong

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

Righteous!
r=me

Comment 8

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a126050ecdb

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4a126050ecdb

Comment 10

[Simona Badau, Desktop QA]

Verified the fix on Ubuntu 13.04 by running the test from the bug description on Spidermonkey built from the latest mozilla-beta-5916e01e5a0a source: no assertion occurs but instead, I get the Reference Error: ParallelArray is not defined. Is this expected?

Comment 11

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

(In reply to Simona B [QA] from comment #10)
> Verified the fix on Ubuntu 13.04 by running the test from the bug
> description on Spidermonkey built from the latest mozilla-beta-5916e01e5a0a
> source: no assertion occurs but instead, I get the Reference Error:
> ParallelArray is not defined. Is this expected?

ParallelArrays, as far as I know, are only enabled on Nightly(?) for now, but I'm not sure so I'd defer to Shu-yu.

Comment 12

[Shu-yu Guo [:shu]]

(In reply to Gary Kwong [:gkw] [:nth10sd] (still catching up on bugmail) from comment #11)
> (In reply to Simona B [QA] from comment #10)
> > Verified the fix on Ubuntu 13.04 by running the test from the bug
> > description on Spidermonkey built from the latest mozilla-beta-5916e01e5a0a
> > source: no assertion occurs but instead, I get the Reference Error:
> > ParallelArray is not defined. Is this expected?
> 
> ParallelArrays, as far as I know, are only enabled on Nightly(?) for now,
> but I'm not sure so I'd defer to Shu-yu.

That's correct, PJS (the now defunct by unremoved ParallelArray API, Array.prototype.*Par methods, and the upcoming Par methods on typed objects) are ifdef'd in only in Nightly.

Comment 13

[Shu-yu Guo [:shu]]

now defunct but unremoved*

Comment 14

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

Simona, you might have to retest on a later build from mozilla-central, since Parallel JS is only enabled by ifdef in Nightly for now.

Comment 15

[Simona Badau, Desktop QA]

(In reply to Gary Kwong [:gkw] [:nth10sd] (still catching up on bugmail) from comment #14)
> Simona, you might have to retest on a later build from mozilla-central,
> since Parallel JS is only enabled by ifdef in Nightly for now.

On the latest Nightly I don't get the same reference error as I do on the latest beta but, instead I get: "uncaught exception: out of memory".
Any thoughts?

Comment 16

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

I think that means it's fixed. I suppose I'm right, Waldo?

Comment 17

[Jeff Walden [:Waldo]]

I don't especially understand the original testcase enough to say what it should or shouldn't do.  But given there was an automated test in the original patch, that would have detected the original issue, and this wasn't a security issue, I don't think this benefits from manual QA resources, or worrying about the expected behavior of the original testcase.

Comment 18

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

Marking VERIFIED per comment 17 as well as the fact that a test was landed.