Bug 36403 - table with cells padded with different widths cause ArrayIndexOutOfBounds exception
Summary: table with cells padded with different widths cause ArrayIndexOutOfBounds exc...
Status: CLOSED FIXED
Alias: None
Product: Fop - Now in Jira
Classification: Unclassified
Component: page-master/layout (show other bugs)
Version: trunk
Hardware: Other other
: P2 normal
Target Milestone: ---
Assignee: fop-dev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-08-28 17:19 UTC by Manuel Mall
Modified: 2012-04-01 07:01 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Mall 2005-08-28 17:19:48 UTC
The fo below throws an IndexOutOfBounds exception when processed through fop. If
one removes the border-collapse attribute no exception is thrown (the generated
PDF then demonstrates other problems related to backgrounds and padding in cells
but thats a different issue I am currently looking at):

    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
      <fo:layout-master-set>
        <fo:simple-page-master master-name="normal" page-width="5in"
page-height="5in">
          <fo:region-body/>
        </fo:simple-page-master>
      </fo:layout-master-set>
      <fo:page-sequence master-reference="normal" white-space-collapse="true">
        <fo:flow flow-name="xsl-region-body">
          <fo:table border-collapse="separate">
            <fo:table-column column-number="1" />
            <fo:table-column column-number="2" />
            <fo:table-column column-number="3" />
            <fo:table-body>
              <fo:table-row>
                <fo:table-cell>
                  <fo:block>Row 1 Col 1</fo:block>
                </fo:table-cell>
                <fo:table-cell margin="0pt" padding="5pt" background-color="blue">
                  <fo:block>Row 1 Col 2</fo:block>
                </fo:table-cell>
                <fo:table-cell margin="0pt" padding="18pt"
background-color="yellow">
                  <fo:block>Row 1 Col 3</fo:block>
                </fo:table-cell>
              </fo:table-row>
            </fo:table-body>
          </fo:table>
          <fo:block>Table with all yellow cells having 18pt padding and all blue
cells having 5pt padding.</fo:block>
        </fo:flow>
      </fo:page-sequence>
    </fo:root>
Comment 1 Manuel Mall 2005-08-29 04:14:19 UTC
Further investigation showed that the problem has nothing to do with the setting
of border-collapse. The fact it doesn't show with border-collapse="collapse" has
simply to do with cell padding being broken in that case. Once I fixed the
padding problem the AIOOB exception is also thrown in the
border-collapse="collapse" case.

The problem is in the TableStepper which attempts a back track on the first row
causing activeRow to be set to -1 (which is an invalid index).
Comment 2 Andreas L. Delmelle 2005-10-25 22:52:12 UTC
Attempting to shed some light on what's happening:
From the debug output, we can derive that TableStepper.getNextStep() has set the variable 
rowBacktrackForLastStep to true (line 513), which in turn leads to 
TableStepper.getCombinedKnuthElementsForRowGroup() to decrease the activeRow index (line 249).

Any subsequent call to TableStepper.getActiveRow() will result in an AIOOBException.

Still looking for the exact reason why the backtrack-switch is flipped to true. Jeremias is probably the 
only one among us who knows what 'Meeeeep!' means ;-)

I'm thinking: either the backtracking itself is out-of-place and shouldn't happen, or the case where the 
active row is the first one should be checked for.
FWIW: simply surrounding TableStepper line 513 with "if (activeRow > 0) { ... }" seems to resolve the 
issue, but whether it suffices (i.e. is more than a quick dirty hack), that I'm not sure about.
Comment 3 Andreas L. Delmelle 2005-10-26 00:50:18 UTC
(just accepting/assigning to fop-dev so everyone can follow further progress)
Comment 4 Jeremias Maerki 2005-10-27 15:21:16 UTC
(In reply to comment #2)
> Attempting to shed some light on what's happening:
> From the debug output, we can derive that TableStepper.getNextStep() has set
the variable 
> rowBacktrackForLastStep to true (line 513), which in turn leads to 
> TableStepper.getCombinedKnuthElementsForRowGroup() to decrease the activeRow
index (line 249).
> 
> Any subsequent call to TableStepper.getActiveRow() will result in an
AIOOBException.
> 
> Still looking for the exact reason why the backtrack-switch is flipped to
true. Jeremias is probably the 
> only one among us who knows what 'Meeeeep!' means ;-)

Hehe. Actually, it should have been "Meeep, meeep!" because Roadrunner always
does a double meeeep! :-) I've replace the comment with a more verbose debug
message and added a few comments in an attempt to shed a little more light on this.

> I'm thinking: either the backtracking itself is out-of-place and shouldn't
happen, or the case where the 
> active row is the first one should be checked for.
> FWIW: simply surrounding TableStepper line 513 with "if (activeRow > 0) { ...
}" seems to resolve the 
> issue, but whether it suffices (i.e. is more than a quick dirty hack), that
I'm not sure about.

You were almost on course. There's an additional aspect here. This case should
actually cause that particular step to be skipped, because borders and padding
allocate more room than the actual step is long which would result in a conflict
situation. I solved this by forcing an INFINITE penalty after the skipped step.
I left a big error sign in there in case a break-before might ruin the fun
there. I'm not sure yet if that's possible at all and what would happen.

Analysis test case on the Wiki documenting a row backtrack situation:
http://wiki.apache.org/xmlgraphics-fop/TableLayout/KnuthElementsForTables/RowBorder2
Hand-written notes with details on the situation (not very verbose, I'm afraid):
http://people.apache.org/~jeremias/fop/NextStepAlgoNotes.pdf

Test case to check this case here:
table_bug36403.xml
(I could actually simplify the case documented in this bug here.)

SVN Revision of the fix:
http://svn.apache.org/viewcvs?rev=328868&view=rev
Comment 5 Glenn Adams 2012-04-01 07:01:12 UTC
batch transition pre-FOP1.0 resolved+fixed bugs to closed+fixed