Bug 66038 – REGRESSION (r91610?): Bing Maps fail to initialize (InvalidOperation: Matrix3D.invert)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

At the <http://maps.bing.com/>, the search field is not functional and the map is not fully rendered and cannot be panned. The JavaScript console shows:
    InvalidOperation: Matrix3D.invert

Occurs in r91611 but not in r91607.

[Radar WebKit Bug Importer]

<rdar://problem/9935288>

[Filip Pizlo]

Here's what is going on:

The DFG JIT (particularly the speculative JIT) may convert a value that is stored in a register into a different format, perhaps even moving it into a different register in the process.  This conversion may either happen in-place (where all subsequent uses of the value end up using the newly converted version of the value) or as a copy.  If it is done as a copy, then everything is fine.  But if it is done in-place, then badness can ensure, particularly if the old (unconverted) version of the value had been spilled.  Subsequent spills and fills of the value will assume that the spilled version of the value is in the same format as the version in the register, which may not be the case.

The principal example of this is converting a JSValue to a double.  The JSValue may be an Int32, a double, or sometjing else.  In the latter case, speculation fails and no conversion is performed.  If it is an Int32, then the value is converted to a double; if it is a double then it is simply unboxed.  But thereafter all code assumes that since the register contains a double then it must be the case that the spilled value is just a boxed double.  Subsequently it is possible that code will be emitted that performs double unboxing on an Int32, which results in rubbish.

The DFG JTI should make an effort to respect discrepencies between the spilled format and the register format, in a way that does not result in registers containing garbage values that lead to programs failing.

A patch is on the way.

[Filip Pizlo]

Created attachment 103740 [details]
the patch

[Gavin Barraclough]

Comment on attachment 103740 [details]
the patch

I think we need to make the same change to JITCodeGenerator::fillDouble?

[Filip Pizlo]

Created attachment 103795 [details]
the patch (fix review)

Added same functionality to JITCodeGenerator::fillDouble()

[Filip Pizlo]

Created attachment 103798 [details]
the patch (fix build)

Fix typos.

[Filip Pizlo]

Created attachment 103803 [details]
the patch

[Gavin Barraclough]

Comment on attachment 103803 [details]
the patch

landing by hand.

[Gavin Barraclough]

Fixed in r92986

[Adam Roben (:aroben)]

Is it not possible to write an automated regression test for this?

[Gavin Barraclough]

Hey Adam,

It would be very difficult to do so.  The bug is triggered through interactions over multiple bytecode operations, including some that are not directly involved in the data flow since the problem is dependent on machine register allocator behavior.

It requires that an integer value is generated by code, spilled, filled, moved to a floating point register, that a unexpected callout from the speculative path occurs (using a silent spill), and that the value is then used again.  Writing a test case that tripped over this would be tricky, and would be likely to be of extremely limited value since it would probably be short lived (any such test case would be hugely fragile to changes in register allocator behavior).

However we have hardened against this kind of bug in the future through Bug 66160, which adds asserts to catch such errors.

[Adam Roben (:aroben)]

OK, thanks for the explanation!

In the future, it would be good to mention why writing a test is not possible in your ChangeLog. That way people won't wonder if you just forgot.

[Gavin Barraclough]

Ah, good point.