1007298 - Assertion failure in js::jit::IonBuilder::pushDOMTypeBarrier

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jesse Ruderman]

Attachment: DOM testcase (requires ion-eager)

With:
  user_pref("javascript.options.ion.unsafe_eager_compilation", true);

Assertion failure: jitinfo->returnType() == JSVAL_TYPE_UNKNOWN || observed->getKnownMIRType() == MIRType_Value || MIRTypeFromValueType(jitinfo->returnType()) == observed->getKnownMIRType(), at js/src/jit/IonBuilder.cpp:6348

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jan de Mooij [:jandem]]

The assert fails because jitInfo->returnType is JSVAL_TYPE_BOOLEAN, but the observed type is MIRType_Object.

Hannes, bug 1001850 probably regressed this; we're optimizing a bound function. Can you take a look?

Comment 3

[Boris Zbarsky [:bzbarsky]]

makeCall/makeCallHelper expect the first arg to be non-null only when there is a single call target.  In this case there isn't.

On the other hand, given that there is not a single call target, how did we end up under inlineSingleCall originally??

Comment 4

[Hannes Verschore [:h4writer]]

As far as I can see this is a bogus assert. A small optimization that hasn't been added yet would also cause this assert to trip [1]. 

The issue is that a "callsite" can be polymorphic. In that case it will generate different paths for the different targets. But the "return value" and "return type" is a combination of all paths. 

That is what happening here. The assert tests the "return type" to be part of what the JITInfo knows the call returns. But in this case the "return type" is poisened by to other polymorphic call. So that is what this assert is complaining about.

So we can just remove the assert. Nothing bad will happen.

[1] This optimization is that when inlining a polymorphic call, we don't remove the covered targets from the input of the fallback block. We don't adjust that information. If we had this, this assert would also trip (unneeded).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Wait.

The assert is there because we're about to make assumptions about the code to generate based on the jitinfo->returnType().

Is the issue that in the polymorphic callsite case we actually end up compiling separate pieces of code for each of the polymorphic pieces, so that making assumptions about the return type based on the jitinfo is still OK even though we've observed return types that are not OK according to the jitinfo type?

Comment 6

[Hannes Verschore [:h4writer]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> Is the issue that in the polymorphic callsite case we actually end up
> compiling separate pieces of code for each of the polymorphic pieces, so
> that making assumptions about the return type based on the jitinfo is still
> OK even though we've observed return types that are not OK according to the
> jitinfo type?

yes

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  And the observed typeset is for after the multiple pieces of code join up again, not for each one?

In that case, I agree that the assert is not ok as it stands.  The goal there was to make sure that the jitinfo return type is not lying...

Is there any way to preserve something like that?

Also, one more thing I don't understand.  The type barrier here is done separately for each of the polymorphic calls, but is there then another type barrier where they join up or something?  That is, at some point "observed" will need to be updated to object-or-boolean, from its current "object" value.

Comment 8

[Hannes Verschore [:h4writer]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> OK.  And the observed typeset is for after the multiple pieces of code join
> up again, not for each one?

Indeed. Since that is kept per callsite, we only know it for all paths, but not for one specific path. Here we have extra information by looking at DOM jit information.

> In that case, I agree that the assert is not ok as it stands.  The goal
> there was to make sure that the jitinfo return type is not lying...
> 
> Is there any way to preserve something like that?

So we have runtime checks that check for objects/string. I think we might want to add some extra runtime checks in debug builds here.

> Also, one more thing I don't understand.  The type barrier here is done
> separately for each of the polymorphic calls, but is there then another type
> barrier where they join up or something?  That is, at some point "observed"
> will need to be updated to object-or-boolean, from its current "object"
> value.

There is no separate barrier at the join point. All paths should do this separately. Now the issue you are mentioning is that we never "observe" the boolean. We don't care anymore. IonMonkey is the last consumer of TI and can ignore/adjust the TI information it receives. So in this case we know the output returns a boolean. We can ignore the TI information and give a result with boolean. It will bubble down as far as it can.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure I made my concern clear...

But looking at the code again, even if we do the unbox infallibly based on the jitinfo, we'll still output a typebarrier if observed doesn't match the jitinfo type.  So I guess things are in fact ok here, except for the assert itself.

Comment 10

[Jan de Mooij [:jandem]]

Boris, I assume our slow paths for DOM getters/methods assert the actual Value we return at runtime matches the jitinfo type?

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: Remove assert

This fixes it indeed.

I indeed didn't notice the pushTypeBarrier afterwards. Which makes sure that we bailout when this happens.

I opened bug 1008138 to remove the pushTypeBarrier. Which is actually not needed anymore. I thought this was already the case. As a result we won't bailout due to this anymore, since we can eagerly support the case JITInfo tells us.

That bug will also add runtime debug checks by using MFilterTypeSet to make sure the information JITInfo gives is actually correct. Which is off course also a concern.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I assume our slow paths for DOM getters/methods assert

Good idea.  ;)  I filed bug 1008236.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8420051 [details] [diff] [review]
Remove assert

r=me

Comment 14

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51f43afc82ad

(Removing security sensitify flag. Just a bogus assertion. Do we want to uplift this? Shouldn't make a difference in optimized builds, only debug builds)

Comment 15

[Jan de Mooij [:jandem]]

Yes we should uplift this. Bogus asserts are annoying for the fuzzers and also for us when debugging a problem on aurora, beta or release.

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 8420051 [details] [diff] [review]
Remove assert

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1001850

User impact if declined: Nothing. (Fuzzers and regression testers will be annoyed).

Testing completed (on m-c, etc.): Just landed

Risk to taking this patch (and alternatives if risky):
This only removes a over-restrictive assert. So no risk.

(This assert tried to test if the type given by a DOM call corresponds to what we say it will be. But it is really the wrong place to do that. In that place it doesn't need to correspond. Bug 1008236 or bug 1008138 will add those checks on the right place again. So we keep having coverage about that.)

String or IDL/UUID changes made by this patch: /

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51f43afc82ad

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f1a6bcbb933

Comment 19

[Paul Silaghi, QA [:pauly]]

Reproduced in Nightly 2014-05-08-mozilla-central-debug.
Verified fixed in Nightly 32.0a1 2014-05-27-mozilla-central-debug, Win 7 x64.