Assertion failure: !v.isObject(), at jit/Recover.cpp

RESOLVED FIXED in mozilla33

Status

()

--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: bbouvier)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla33
x86_64
macOS
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 8450707 [details]
stack

function f(x, y) {
    !y != (y != Math.abs())
}
function g(x) {
    f(f(), (function() {}))
}
g(function(){})
g()

asserts js debug shell on m-c changeset 06e9a27a6fcc with --ion-eager --ion-offthread-compile=off at Assertion failure: !v.isObject(), at jit/Recover.cpp

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140630233053" and the hash "db4f6e215872".
The "bad" changeset has the timestamp "20140701004453" and the hash "881e7b3d101f".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=db4f6e215872&tochange=881e7b3d101f

Benjamin, is bug 1013821 a likely regressor?
Flags: needinfo?(benj)
(Assignee)

Comment 1

5 years ago
Nice catch! So after discussion with nbp, the assertion is silly and we can just make canRecoverOnBailout return true in all cases, as the instruction isn't effectful (see MNot::getAliasSet).

Guillaume, this error is a fallout from the RNot bug. It's my fault as I didn't think of this particular case (look at the output of the iongraph if you want to see what goes wrong). As it's a really small patch to do, would you be interested in doing it? If so, please modify the code and add the testcase provided in this bug in your patch as well. Otherwise, I'll take care of it :)
Flags: needinfo?(benj) → needinfo?(layus.on)
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> [...] the assertion is silly and we can just make canRecoverOnBailout 
> return true in all cases, as the instruction isn't effectful

Ahah ! So you finally surrender to my arguments :-)

> (see MNot::getAliasSet).

Huh ?

> Guillaume, this error is a fallout from the RNot bug. It's my fault as I
> didn't think of this particular case (look at the output of the iongraph if
> you want to see what goes wrong). As it's a really small patch to do, would
> you be interested in doing it? If so, please modify the code and add the
> testcase provided in this bug in your patch as well. Otherwise, I'll take
> care of it :)

I would be glad to work on this.
As I will be on holiday starting from Sunday (2014-07-06), feel free to fix it yourself if I was not able to do it before.
Flags: needinfo?(layus.on)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1dc6b294800d).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:bisectfix]
(Assignee)

Comment 4

5 years ago
Just ran bisection by hand:

The first good revision is:
changeset:   192441:0f85fbed35dc
user:        Hannes Verschore <hv1989@gmail.com>
date:        Fri Jul 04 19:43:24 2014 +0200
summary:     Bug 1019983 - Don't optimize compare based on baseline caches when more types are seen than present in the cache, r=jandem

So, this patch made the Compare a generic one (inputs are Values) while the Compare was specialized as a double compare before (the parameter y was converted into a double). As the compare is now generic, there is no bailout anymore. However, as the issue can still happen in other situations where a Compare isn't involved, I'll work on a patch.
Whiteboard: [jsbugmon:bisectfix]
(Assignee)

Comment 5

5 years ago
Created attachment 8451477 [details] [diff] [review]
bug1034400.patch

With another baked test case that shows the issue.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8451477 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8451477 [details] [diff] [review]
bug1034400.patch

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

::: js/src/jit-test/tests/ion/bug1034400.js
@@ +7,5 @@
> +    }
> +}
> +
> +var countH = 0;
> +function h() {

s/h/uceFault/
Attachment #8451477 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/3708aa158f68
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.