Bug 66038 - REGRESSION (r91610?): Bing Maps fail to initialize (InvalidOperation: Matrix3D.invert)
Summary: REGRESSION (r91610?): Bing Maps fail to initialize (InvalidOperation: Matrix3...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Nobody
URL: http://maps.bing.com
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-08-11 00:09 PDT by mitz
Modified: 2011-08-15 11:08 PDT (History)
5 users (show)

See Also:


Attachments
the patch (2.15 KB, patch)
2011-08-11 23:26 PDT, Filip Pizlo
barraclough: review-
Details | Formatted Diff | Diff
the patch (fix review) (2.86 KB, patch)
2011-08-12 12:40 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (fix build) (2.86 KB, patch)
2011-08-12 12:43 PDT, Filip Pizlo
barraclough: review+
Details | Formatted Diff | Diff
the patch (2.74 KB, patch)
2011-08-12 13:12 PDT, Filip Pizlo
barraclough: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2011-08-11 00:09:18 PDT
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.
Comment 1 Radar WebKit Bug Importer 2011-08-11 00:09:36 PDT
<rdar://problem/9935288>
Comment 2 Filip Pizlo 2011-08-11 23:13:06 PDT
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.
Comment 3 Filip Pizlo 2011-08-11 23:26:24 PDT
Created attachment 103740 [details]
the patch
Comment 4 Gavin Barraclough 2011-08-12 00:04:54 PDT
Comment on attachment 103740 [details]
the patch

I think we need to make the same change to JITCodeGenerator::fillDouble?
Comment 5 Filip Pizlo 2011-08-12 12:40:58 PDT
Created attachment 103795 [details]
the patch (fix review)

Added same functionality to JITCodeGenerator::fillDouble()
Comment 6 Filip Pizlo 2011-08-12 12:43:28 PDT
Created attachment 103798 [details]
the patch (fix build)

Fix typos.
Comment 7 Filip Pizlo 2011-08-12 13:12:10 PDT
Created attachment 103803 [details]
the patch
Comment 8 Gavin Barraclough 2011-08-12 13:30:38 PDT
Comment on attachment 103803 [details]
the patch

landing by hand.
Comment 9 Gavin Barraclough 2011-08-12 13:33:40 PDT
Fixed in r92986
Comment 10 Adam Roben (:aroben) 2011-08-15 07:12:32 PDT
Is it not possible to write an automated regression test for this?
Comment 11 Gavin Barraclough 2011-08-15 10:55:34 PDT
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.
Comment 12 Adam Roben (:aroben) 2011-08-15 10:57:03 PDT
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.
Comment 13 Gavin Barraclough 2011-08-15 11:08:33 PDT
Ah, good point.