[TIMOB-20206] Android : TiCompositeLayout's viewSorter does not abide by Comparator's contract
| GitHub Issue | n/a |
|---|---|
| Type | Bug |
| Priority | Critical |
| Status | Closed |
| Resolution | Fixed |
| Resolution Date | 2016-09-06T06:00:49.000+0000 |
| Affected Version/s | n/a |
| Fix Version/s | Release 6.0.2 |
| Components | Android |
| Labels | n/a |
| Reporter | Narayan K |
| Assignee | Ashraf Abu |
| Created | 2016-01-05T17:56:04.000+0000 |
| Updated | 2017-01-31T18:30:59.000+0000 |
Description
I'm writing in from the Android team. The "N" release of Android is moving over to using OpenJdk as its implementation for java.* libraries. One of the issues that came up in our testing a crash that looks like this :
java.lang.IllegalStateException: Ambiguous Z-Order
at org.appcelerator.titanium.view.TiCompositeLayout$1.compare(TiCompositeLayout.java:164)
at org.appcelerator.titanium.view.TiCompositeLayout$1.compare(TiCompositeLayout.java:127)
at java.util.TreeMap.compare(TreeMap.java:1190)
at java.util.TreeMap.put(TreeMap.java:532)
at java.util.TreeSet.add(TreeSet.java:255)
at org.appcelerator.titanium.view.TiCompositeLayout.onLayout(TiCompositeLayout.java:491)
at android.view.View.layout(View.java:16945)
at android.view.ViewGroup.layout(ViewGroup.java:5516)
The issue here is their Comparator expects that its arguments are never equal (i.e, none of its code paths return 0). That seems pretty brittle and contrary to the documented contract.
It fails on OpenJdk because of this snippet of code in TreeMap#put :
TreeMapEntry t = root;
if (t == null) {
compare(key, key); // type (and possibly null) check <--- [BUG HERE] we're deliberately comparing a view with itself for the side effects : null & type check
.....
I will put in a workaround for the upcoming release, but It will be reverted in a future Android release.
[~kamath.narayan@gmail.com] Thanks for the ticket! Is this something that only occurs with "N" (with the OpenJDK implementation)?
Yes, this is new with the OpenJDK TreeMap implementation in N
Hello all - have you had any luck fixing this issue ? The Android N preview is now public and contains a platform workaround. We would like to remove the workaround at some point, which would leave apps that use this class broken.
[~kamath.narayan@gmail.com] Thanks for the ping. We will be working on this and testing it on N. :)
In the change cited here [1], we have reverted the workaround I mentioned in my earlier comment. All applications using TiCompositeLayout will break in a future android release unless this bug is fixed. [1] https://android-review.googlesource.com/#/c/257511/1
[~kamath.narayan@gmail.com] Thanks for the update! So basically beyond N will break this?
To refer to https://android-review.googlesource.com/#/c/257511/1/ojluni/src/main/java/java/util/TreeMap.java
hi - yes, that's right.
Thanks for replying! Appreciate it a whole lot.
For ref: http://grepcode.com/file_/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/util/Comparator.java/?v=source
/** * Compares its two arguments for order. Returns a negative integer, * zero, or a positive integer as the first argument is less than, equal * to, or greater than the second.<p> * * In the foregoing description, the notation * <tt>sgn(</tt><i>expression</i><tt>)</tt> designates the mathematical * <i>signum</i> function, which is defined to return one of <tt>-1</tt>, * <tt>0</tt>, or <tt>1</tt> according to whether the value of * <i>expression</i> is negative, zero or positive.<p> * * The implementor must ensure that <tt>sgn(compare(x, y)) == * -sgn(compare(y, x))</tt> for all <tt>x</tt> and <tt>y</tt>. (This * implies that <tt>compare(x, y)</tt> must throw an exception if and only * if <tt>compare(y, x)</tt> throws an exception.)<p> * * The implementor must also ensure that the relation is transitive: * <tt>((compare(x, y)>0) && (compare(y, z)>0))</tt> implies * <tt>compare(x, z)>0</tt>.<p> * * Finally, the implementor must ensure that <tt>compare(x, y)==0</tt> * implies that <tt>sgn(compare(x, z))==sgn(compare(y, z))</tt> for all * <tt>z</tt>.<p> * * It is generally the case, but <i>not</i> strictly required that * <tt>(compare(x, y)==0) == (x.equals(y))</tt>. Generally speaking, * any comparator that violates this condition should clearly indicate * this fact. The recommended language is "Note: this comparator * imposes orderings that are inconsistent with equals." * * @param o1 the first object to be compared. * @param o2 the second object to be compared. * @return a negative integer, zero, or a positive integer as the * first argument is less than, equal to, or greater than the * second. * @throws ClassCastException if the arguments' types prevent them from * being compared by this comparator. */ int compare(T o1, T o2);PR: https://github.com/appcelerator/titanium_mobile/pull/8287 [~cwilliams] for your review.
6_0_X: https://github.com/appcelerator/titanium_mobile/pull/8793
Test Tuesday 31st January 2017 ENV: Thursday 26th January 2017: ENV: MacOS:10.12.1 XCODE: 8.2.1 GM (golden master) APPC CLI Core: 6.1.0 APPC CLI NPM: 4.2.8 SDK: 6.0.2.v20170130045621 Studio build: 4.8.1.201612050850 NPM: 2.15.9 Node: 4.5.0 Device: Google Pixel Android Version: 7.1 Step 1) Create a new classic app in studio Step 2) Run the classic app with the latest 6.0.2 build on a android phone with 7.0 or greater Step 3) Verify that the app does not crash After completing the steps above i can verify that the fix is ok
In Addition to the above here is the app.js code i tested to verify this ticket
var win = Ti.UI.createWindow({backgroundColor: 'gray'}), view = Ti.UI.createView({backgroundColor: 'red', width: '66%', height: '66%'}); win.add(view); win.add(view); win.open();