Closed
Bug 895792
Opened 11 years ago
Closed 11 years ago
Assertion failure: ~x > x (can't round up -- will overflow!), at dist/include/mozilla/MathAlgorithms.h
Categories
(Core :: MFBT, defect)
Tracking
()
VERIFIED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox24 | --- | unaffected |
firefox25 | --- | fixed |
People
(Reporter: gkw, Assigned: Waldo)
References
Details
(Keywords: assertion, regression, testcase)
Attachments
(3 files)
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.
Flags: needinfo?(shu)
Comment 1•11 years ago
|
||
Like the other GC fuzz test, I can't reproduce this. 32bit or 64bit, --enable-debug --enable-optimize, --enable-threadsafe. :/
Flags: needinfo?(shu)
Reporter | ||
Comment 2•11 years ago
|
||
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?
Blocks: 891177
Flags: needinfo?(jwalden+bmo)
Updated•11 years ago
|
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee: general → jwalden+bmo
Flags: needinfo?(jwalden+bmo)
Comment 4•11 years ago
|
||
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #781910 -
Flags: review?(terrence)
Assignee | ||
Comment 6•11 years ago
|
||
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.
Group: core-security
Assignee | ||
Updated•11 years ago
|
Component: JavaScript Engine → MFBT
Comment 7•11 years ago
|
||
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
Attachment #781910 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla25
Comment 9•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
status-firefox24:
--- → unaffected
status-firefox25:
--- → fixed
Comment 10•11 years ago
|
||
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?
Flags: needinfo?(jwalden+bmo)
Reporter | ||
Comment 11•11 years ago
|
||
(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.
Flags: needinfo?(jwalden+bmo) → needinfo?(shu)
Comment 12•11 years ago
|
||
(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.
Flags: needinfo?(shu)
Comment 13•11 years ago
|
||
now defunct but unremoved*
Reporter | ||
Comment 14•11 years ago
|
||
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.
Flags: needinfo?(simona.marcu)
Comment 15•11 years ago
|
||
(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?
Flags: needinfo?(simona.marcu)
Reporter | ||
Comment 16•11 years ago
|
||
I think that means it's fixed. I suppose I'm right, Waldo?
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 17•11 years ago
|
||
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.
Flags: needinfo?(jwalden+bmo)
Reporter | ||
Comment 18•11 years ago
|
||
Marking VERIFIED per comment 17 as well as the fact that a test was landed.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•