Assertion failure: MIR instruction returned object with unexpected type, at jit/IonMacroAssembler.cpp:1219

VERIFIED FIXED in Firefox 30



JavaScript Engine: JIT
4 years ago
3 years ago


(Reporter: decoder, Assigned: nmatsakis)


(Blocks: 1 bug, {assertion, sec-critical, testcase})

assertion, sec-critical, testcase
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox27 disabled, firefox28 disabled, firefox29 disabled, firefox30 fixed, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected)


(Whiteboard: [jsbugmon:update,ignore])


(2 attachments)



4 years ago
The following testcase asserts on mozilla-central revision c049cb230d77 (run with --fuzzing-safe --ion-check-range-analysis --ion-compile-try-catch --ion-eager):

var N = 100;
var T = TypedObject;
var Point = new T.StructType({x: T.uint32, y: T.uint32, z: T.uint32});
var PointArray = Point.array();
var array = new PointArray(N);
for (var i = 0; i < N; i++) {
  array[i].x = i + 0;
  try {} catch(e) {}

Comment 1

4 years ago
Created attachment 8347736 [details]
[crash-signature] Machine-readable crash signature
Niko, this is failing the new type checks I added in bug 949475.

It looks like we have a MNewDerivedTypedObject with an empty resultTypeSet, but it's returning something.
Flags: needinfo?(nmatsakis)


4 years ago
Whiteboard: [jsbugmon:update,bisect]


4 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 3

4 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
user:        Jan de Mooij
date:        Sat Dec 14 14:32:35 2013 +0100
summary:     Bug 949475 - Add some debug-only sanity checks. r=bhackett

This iteration took 346.742 seconds to run.


4 years ago
Assignee: general → nobody
QA Contact: general

Comment 4

4 years ago
This is related to bug 943852, I believe.
Flags: needinfo?(nmatsakis)
Bug 953373 may be related.
Keywords: sec-critical
Niko, can we get somebody assigned here? This is a very generic assert and fixing it makes it easier for the fuzzers to find other, unrelated bugs.
Flags: needinfo?(nmatsakis)

Comment 7

4 years ago
Jan -- yes, pnkfelix and I have been discussing the right fix off and on, I will attempt to tackle soon. I'm leaving the needinfo to keep this on my radar.


4 years ago
Assignee: nobody → nmatsakis
Flags: needinfo?(nmatsakis)
Group: javascript-core-security
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-firefox27: --- → disabled
status-firefox28: --- → disabled
status-firefox29: --- → affected
status-firefox-esr24: --- → unaffected
Okay so I cannot replicate via the command line flags that were originally posted on the bug, but I can if I add |--ion-parallel-compile=off| in addition to |--ion-eager|

I am looking further now.

Comment 9

4 years ago
I am able to reproduce using --ion-parallel-compile=off. I have been doing some extensive refactorings and I think I have the ability to do the "right" fix here -- which (I believe) is for us to compute the complete result type set and use that, rather than using the observed type set, which can be incomplete.

Comment 10

4 years ago
Actually, so long as the prototypes on type descriptors are user mutable (which they currently are) I think my plan won't work -- if the user mutates the prototype for a type descriptor, this will cause a new TI type object to be used for later instances (since the TI type object is keyed by class/prototype). Would be nicer if the prototypes for type descriptors were immutable, though I don't know if this is in the cards.

I have therefore experimented with various possible fixes, all of which seem to work.

1. Add a type barrier whenever we create a derived typed object. This is unfortunate but perhaps of little consequence in real code -- first, I expect most derived typed objects to be optimized away (and hopefully the barriers would be optimized away too -- I haven't checked yet), but second I would expect that in 'normal' code these type sets will have only a single entry.

2. Do not set resultTypeSet. This solves the problem but (I believe) impedes further optimization because further uses of this derived typed object are not aware of its type. I am conducting further experiments, because I'm not sure I 100% understand how IonBuilder/TI/etc interact. It might be possible to reclaim some of the optimizations by modifying the routine that identifies the TypeRepresentationSet from an MDefinition to have special treatment for a MDerivedTypedObject. This at least recovers optimal behavior for a sequence of accessors like `a.b.c`, but (I believe) it will optimize more poorly in the face of loops or phis.

For the time being, I am inclined to leave in the type barrier until perf measurements suggest it is a problem.

Comment 11

4 years ago
OK, I understand better now and can clarify the impact on optimization of removing the resultTypeSet:

As I initially thought, if we do not set the resultTypeSet at all, then we are giving less information to consumers of the derived typed object and this can hinder optimization. I wasn't observing this at first because the routine which goes from a MDefinition* to a TypeRepresentationSet already had an optimization which, if its input was a MNewDerivedTypedObject, extracted the TypeRepresentationSet directly. Otherwise, it would derive the TypeRepresentationSet from the resultTypeSet. This is in fact the optimization I was referring to when I said "it might be possible to reclaim some of the optimizations".

Long story short, I'm inclined to add the type barrier for now.

Comment 12

4 years ago
Created attachment 8368809 [details] [diff] [review]

1. Consolidate some common code paths
2. Insert a type barrier after creating derived typed objects
Attachment #8368809 - Flags: review?(jdemooij)
Comment on attachment 8368809 [details] [diff] [review]

Review of attachment 8368809 [details] [diff] [review]:


::: js/src/jit/IonBuilder.cpp
@@ +6729,5 @@
>      if (!checkTypedObjectIndexInBounds(elemSize, obj, index, &indexAsByteOffset, objTypeReprs))
>          return false;
> +    return pushScalarLoadFromTypedObject(emitted, obj,
> +                                         indexAsByteOffset, elemTypeRepr);

Nit: fits on one line.
Attachment #8368809 - Flags: review?(jdemooij) → review+

Comment 14

4 years ago

Comment 15

4 years ago


4 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]

Comment 16

4 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 579cf46bc21e).
landed on central as
Last Resolved: 4 years ago
status-firefox30: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla30


4 years ago

Comment 18

4 years ago
JSBugMon: This bug has been automatically verified fixed.
I assume we want to uplift this? Can you look into creating an aurora fix and ask for approval?
Flags: needinfo?(nmatsakis)
TypedObject is only enabled on Nightly I think. Maybe Aurora too but not beta/release.
(In reply to Jan de Mooij [:jandem] from comment #20)
> TypedObject is only enabled on Nightly I think. Maybe Aurora too but not
> beta/release.

According to flags: disabled on FF27/FF28, affected FF29 and fixed FF30. So aurora is still affected.

Comment 22

4 years ago
I don't believe aurora is affected -- Typed Objects can only be created in Nightly builds.
Flags: needinfo?(nmatsakis)
(In reply to Niko Matsakis [:nmatsakis] from comment #22)
> I don't believe aurora is affected -- Typed Objects can only be created in
> Nightly builds.

Setting flag for aurora builds to disabled.
status-firefox29: affected → disabled
Group: javascript-core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.