Closed Bug 1004527 Opened 10 years ago Closed 10 years ago

Assertion failure: !val.isMagic(), at jsobj.cpp:5489

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox32 --- fixed

People

(Reporter: decoder, Unassigned)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

The following testcase asserts on mozilla-central revision b227a707080f (run with --fuzzing-safe --ion-eager):


var { ArrayType, StructType, uint32 } = TypedObject;
var L = 1024;
var Matrix = uint32.array(L, 2);
var matrix = new Matrix();
evaluate("for (var i = 0; i < L; i++) matrix[i][0] = (function d() {});", { compileAndGo : true });
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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/d34458e80bcb
user:        Shu-yu Guo
date:        Thu Apr 24 01:59:36 2014 -0700
summary:     Bug 716647 - Part 1: Introduce JS_OPTIMIZED_OUT magic for optimized out slots and teach Debugger about them. (r=jandem)

This iteration took 1.704 seconds to run.
Needinfo from shu based on comment 2.
Flags: needinfo?(shu)
What's going on is that |matrix[i][0]| is initially compiled as |MNewDerivedTypedObject|, but TI is smart enough to figure out that since matrix is definitely a TypedObject, we can do the assignment into |matrix[i][0]| without creating an intermediate TypedObject. So now |MNewDerivedTypedObject| is dead, as its only use is by a resume point. The problem is that in this case, the resume point use is a real use -- if we bail out, we *do* need to create a new derived TypedObject corresponding to |matrix[i][0]|.

We obviously don't want to actually emit code for |MNewDerivedTypedObject|, since that would defeat the purpose of the optimization. Luckily Nicolas landed recover instructions recently, and I'm currently seeing if I can make |MNewDerivedTypedObject| recover on bailout.

This is an existing bug that should've been caught by differential analysis, I suppose. Also CC'ing Niko.
Flags: needinfo?(shu)
See previous comment for explanation.

Nicolas, nice work on recover instructions! Am I using them right here?
Attachment #8416240 - Flags: review?(nmatsakis)
Attachment #8416240 - Flags: feedback?(nicolas.b.pierron)
Comment on attachment 8416240 [details] [diff] [review]
Don't eliminate MNewDerivedTypedObject from resume points and recover them on bailout.

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

This looks great. nbp -- the recover framework looks really nice.

::: js/src/jit-test/tests/TypedObject/bug1004527.js
@@ +1,5 @@
> +var { ArrayType, StructType, uint32 } = TypedObject;
> +var L = 1024;
> +var Matrix = uint32.array(L, 2);
> +var matrix = new Matrix();
> +evaluate("for (var i = 0; i < L; i++) matrix[i][0] = (function d() {});", { compileAndGo : true });

can you make the test conditional on TypedObject being defined?
Attachment #8416240 - Flags: review?(nmatsakis) → review+
Comment on attachment 8416240 [details] [diff] [review]
Don't eliminate MNewDerivedTypedObject from resume points and recover them on bailout.

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

::: js/src/jit/Recover.cpp
@@ +184,5 @@
> +bool
> +RNewDerivedTypedObject::recover(JSContext *cx, SnapshotIterator &iter) const
> +{
> +    Rooted<SizedTypeDescr *> descr(cx, &iter.read().toObject().as<SizedTypeDescr>());
> +    Rooted<TypedObject *> owner(cx, &iter.read().toObject().as<TypedObject>());

be aware that objects are really dangerous things to manipulate in Recover Instructions.  The reason is that an object can be mutated which cause the invalidation of the script.  Once we come back to instance on the stack we will bails and use the recover instruction.  The recover instruction need to see the state before the object mutation, to ensure that we effectively resume the same thing.

If we can guarantee that these objects would always remain immutable (or not escaped), then this optimization is fine.
Are these 2 object arguments mutable?  If not always, this should be ensured in canRecoverOnBailout().
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> Comment on attachment 8416240 [details] [diff] [review]
> Don't eliminate MNewDerivedTypedObject from resume points and recover them
> on bailout.
> 
> Review of attachment 8416240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/Recover.cpp
> @@ +184,5 @@
> > +bool
> > +RNewDerivedTypedObject::recover(JSContext *cx, SnapshotIterator &iter) const
> > +{
> > +    Rooted<SizedTypeDescr *> descr(cx, &iter.read().toObject().as<SizedTypeDescr>());
> > +    Rooted<TypedObject *> owner(cx, &iter.read().toObject().as<TypedObject>());
> 
> be aware that objects are really dangerous things to manipulate in Recover
> Instructions.  The reason is that an object can be mutated which cause the
> invalidation of the script.  Once we come back to instance on the stack we
> will bails and use the recover instruction.  The recover instruction need to
> see the state before the object mutation, to ensure that we effectively
> resume the same thing.
> 
> If we can guarantee that these objects would always remain immutable (or not
> escaped), then this optimization is fine.
> Are these 2 object arguments mutable?  If not always, this should be ensured
> in canRecoverOnBailout().

Discussed with Niko briefly on IRC and concluded that the operands are indeed immutable.
Attachment #8416240 - Flags: feedback?(nicolas.b.pierron) → feedback+
https://hg.mozilla.org/mozilla-central/rev/e66519a65224
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1006899
Blocks: 716647
Keywords: regression
You need to log in before you can comment on or make changes to this bug.