Bugzilla – Bug 970
Java 1.7 IllegalStateException: Two cells in effect cannot start on the same column
Last modified: 2013-04-17 17:16:50 CEST
I’m running the validator locally using Java 1.7.0_04, which is the default on Mac OS 10.8.2. This sample file: <!DOCTYPE html> <html> <head> <meta charset=UTF-8> <title>Table</title> </head> <body> <table> <tr> <th rowspan=2>One</th> <th>Two</th> </tr> <tr> <th>Three</th> </tr> </table> </body> </html> Throws this exception: java.lang.IllegalStateException: Two cells in effect cannot start on the same column, so this should never happen! at org.whattf.checker.table.VerticalCellComparator.compare(VerticalCellComparator.java:53) at org.whattf.checker.table.VerticalCellComparator.compare(VerticalCellComparator.java:34) at java.util.TreeMap.compare(TreeMap.java:1188) at java.util.TreeMap.put(TreeMap.java:531) at java.util.TreeSet.add(TreeSet.java:255) at org.whattf.checker.table.RowGroup.cell(RowGroup.java:100) The problem seems to be that the internal implementation of TreeSet or TreeMap has changed for Java 7, and sometimes the comparator is called with both arguments equal. This patch fixes the problem by checking whether cell0 == cell1. If they are, no exception is thrown, but if there is a problem, additional debugging information is included in the exception message. diff -r c3b787c0705d non-schema/java/src/org/whattf/checker/table/VerticalCellComparator.java --- a/non-schema/java/src/org/whattf/checker/table/VerticalCellComparator.java Mon Mar 11 05:38:54 2013 +0900 +++ b/non-schema/java/src/org/whattf/checker/table/VerticalCellComparator.java Mon Mar 11 20:39:11 2013 -0600 @@ -49,8 +49,16 @@ return -1; } else if (cell0.getLeft() > cell1.getLeft()) { return 1; + } else if (cell0 == cell1) { + return 0; } else { - throw new IllegalStateException("Two cells in effect cannot start on the same column, so this should never happen!"); + throw new IllegalStateException("Two cells in effect cannot start on the same column, so this should never happen!\n" + + "cell0 from line " + cell0.getLineNumber() + + ", bottom=" + cell0.getBottom() + + ", left=" + cell0.getLeft() + "\n" + + "cell1 from line " + cell1.getLineNumber() + + ", bottom=" + cell1.getBottom() + + ", left=" + cell1.getLeft()); } } } Cheers, Andrew
We have other cases where this patch is not enough (still end up with the exception). In any case, the comparator should be implemented completely and define a complete order for Cell. I'm not entirely sure of what defines the entire state for a Cell, but my comparator would look like that: [[ public final int compare(Cell cell0, Cell cell1) { int bottom = cell0.getBottom() - cell1.getBottom(); if (bottom != 0) return bottom; int left = cell0.getLeft() - cell1.getLeft(); if (left != 0) return left; int right = cell0.getRight() - cell1.getRight(); if (right != 0) return right; int columnNumber = cell0.getColumnNumber() - cell1.getColumnNumber(); if (columnNumber != 0) return columnNumber; int lineNumber = cell0.getLineNumber() - cell1.getLineNumber(); return lineNumber; } ]] It's basically a lexicographic order on the accessors getBottom, getLeft, getRight, getColumnNumber and finally getLineNumber. This looks ok for us on actual data we see at W3C. Alexandre.
Hi! What do I need to do to get this patch to get into the main release?
Hi! What do I need to do to get this patch into the main release?
(In reply to comment #3) > Hi! What do I need to do to get this patch into the main release? Sorry about the delay here. To integrate your patch, I need a statement from you that you license your patches to Validator.nu under the same license as the file being patched (MIT license in the case of this file) and a statement if you don't include a copyright notice of yours in the patch, you are OK without having your copyright notice in the license header. (In reply to comment #1) > We have other cases where this patch is not enough (still end up with the > exception). That's worrying and suggests that there's a bug in the validator code that goes beyond failing to account for TreeMap comparing an object with itself. Do you have a test case that triggers the exception with the patch from comment 0 applied?
Hmm, let me do this: I hereby transfer the copyright for all of Comment #0 and the source code contained in it to Henri Sivonen. Now that you own the patch, you are free to use any license you wish, with any copyright notice you choose. Cheers, Andrew
Thank you. Landed: https://bitbucket.org/validator/syntax/commits/1a92b27841ed75587c640018504486c47e9e2cc3 (And deployed although the deployment is on OpenJDK6.) If the problem from comment 1 still shows up, please follow up with steps to reproduce.