Bug 235368

Comment 1 Stefan Röck 2008-06-03 11:34:42 EDT

Created attachment 103345 [details]
Snippet

Comment 2 Rüdiger Herrmann 2008-06-04 10:12:04 EDT

Created attachment 103552 [details]
Bugfix

Comment 3 Benjamin Muskalla 2008-06-04 19:51:30 EDT

Sorry I've some problems reproducing the exact bug with the attached snippet. (I'm aware I should use the mousewheel)
One thing I can see without that patch:
* start snippet
* scroll down to the end by dragging the scrollbar thumb
* table populates data and increases item count
* scroll down to the end

This produces the following output:
Invalid index 260
Invalid index 261
....
Invalid index 298
Invalid index 299

With the patch in place the same happens but with the first "scroll to bottom".
Not yet sure if i hit the same bug or something similar

Comment 4 Rüdiger Herrmann 2008-06-05 03:52:34 EDT

 (In reply to comment #3)
 
I cannot reproduce what you describe. I wheel-scrolled slow and fast but all works as expected.
However, to me this appears to be a different thing.

To get some more insight on what you discovered, could you extend the sys-out like this:
  System.err.println( "Invalid index " + index + " itemCount: " + tableViewer.getTable().getItemCount() );
Thus we could see if the index exceeds the total number of items known by the table widget.
 
> Sorry I've some problems reproducing the exact bug with the attached snippet.
> (I'm aware I should use the mousewheel)
> One thing I can see without that patch:
> * start snippet
> * scroll down to the end by dragging the scrollbar thumb
> * table populates data and increases item count
> * scroll down to the end
> 
> This produces the following output:
> Invalid index 260
> Invalid index 261
> ....
> Invalid index 298
> Invalid index 299
> 
> With the patch in place the same happens but with the first "scroll to bottom".
> Not yet sure if i hit the same bug or something similar

Comment 5 Stefan Röck 2008-06-05 05:43:24 EDT

Hi,
please, uncomment these lines in the snippet where the output is written, they are a relict of an old test and are WRONG there. The updateElement() method should look like this:

public void updateElement(int index){
    tableViewer.replace(getModel().get(index), index);
    if (index > getModel().size() - PREFETCH_TRESHOLD && (getModel().size() < MAX_ENTRIES)) {
        // simulate loading the next page of data from db
        int approxRecordCount = addElementsToModel();
        
        System.out.println("approx. record count: " + approxRecordCount);
        tableViewer.setItemCount(approxRecordCount);
        updateRowCountStatus(tableViewer);
    }

}


The check index < getModel().size() was wrong because in this snippet, the table always has more rows than the model has items, which lead to the irritating "invalid index" output.

Comment 6 Stefan Röck 2008-06-05 08:01:19 EDT

Hi Rüdiger,

unfortunately the problem persists. The reasons seems to be that the queried index somehow gets -1 when scrolling fastly down. This can cause Array index out of bounds errors or SWT errors at a lot of places in the code. 
For testing, I tried to add checks e.g. in  org.eclipse.jface.viewers.AbstractTableViewer$VirtualManager.resolveElement but after doing that, the error is thrown somewhere else (e.g. org.eclipse.swt.widgets.Table.getItem).

So, I am not sure if that's a good approach. Do you see any chance to find out where this -1 index comes from and maybe elimnate it (either on qx side or at least in the LCA)?

Example error:
java.lang.ArrayIndexOutOfBoundsException: -1
	at org.eclipse.jface.viewers.AbstractTableViewer$VirtualManager.resolveElement(AbstractTableViewer.java:94)
	at org.eclipse.jface.viewers.AbstractTableViewer$1.handleEvent(AbstractTableViewer.java:64)
	at org.eclipse.swt.internal.widgets.UntypedEventAdapter.dispatchEvent(UntypedEventAdapter.java:312)
	at org.eclipse.swt.internal.widgets.UntypedEventAdapter.update(UntypedEventAdapter.java:145)
	at org.eclipse.swt.internal.widgets.SetDataEvent.dispatchToObserver(SetDataEvent.java:41)
	at org.eclipse.rwt.internal.events.Event.processEvent(Event.java:44)
	at org.eclipse.swt.events.TypedEvent.processEvent(TypedEvent.java:114)
	at org.eclipse.swt.widgets.Table$1.run(Table.java:2141)
	at org.eclipse.rwt.lifecycle.ProcessActionRunner.executeNext(ProcessActionRunner.java:57)
	at org.eclipse.rwt.internal.lifecycle.RWTLifeCycle.readAndDispatch(RWTLifeCycle.java:241)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:682)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2311)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2274)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:373)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:154)
	at de.cas.teamcrm.application.ApplicationEntryPoint.createWorkBench(ApplicationEntryPoint.java:194)
	at de.cas.teamcrm.application.ApplicationEntryPoint.createUI(ApplicationEntryPoint.java:165)
	at org.eclipse.rwt.internal.lifecycle.EntryPointManager.createUI(EntryPointManager.java:92)
	at org.eclipse.rwt.internal.lifecycle.RWTLifeCycle.createUI(RWTLifeCycle.java:230)
	at org.eclipse.rwt.internal.lifecycle.RWTLifeCycle$UIThreadController.run(RWTLifeCycle.java:116)
	at java.lang.Thread.run(Thread.java:595)

Comment 7 Stefan Röck 2008-06-05 11:20:29 EDT

Created attachment 103757 [details]
Patch for solving Array Index Out of bounds errors for the given scenario (see first attachment)

Note: This patch comprises the first patch from Rüdiger.

Comment 8 Rüdiger Herrmann 2008-06-06 05:15:54 EDT

Created attachment 103908 [details]
Adjusted version of 'Patch' (3rd attachment)

- removed sys-out
- added test case
- changed code format

Comment 9 Stefan Röck 2008-06-06 06:39:55 EDT

Sorry for the formatting issues - maybe you can link the RAP projects with your formatters somewhen?

As far as I can see, you changed the checkData() method again. In my version the index check is done twice (before and after firing the event). This was necessary in my tests (SWT error otherwise). I guess, this is because the event listeners implementation changes the number of items of the table (as done in updateElement() in the content provider).

Comment 10 Rüdiger Herrmann 2008-06-06 10:55:26 EDT

Right, we should have the formatter in the project settings. meanwhile have a look here: 
  http://wiki.eclipse.org/RAP/CodingConventions

The postcondition that checks if the table or the item was disposed is taken as-is from SWT. This is why I again changed the index-range checks.

Tests with the snippet from the first attachment all went well.
Can you reproduce the SWT error with the snippet or did you have other code running?

Comment 11 Stefan Röck 2008-06-09 02:40:14 EDT

I can reproduce the SWT error with a slightly modified snippet (see attachment).
Steps:
1. Click into table
2. Hold down the PageDown-key until the end of list is reached (should be 314 records only)

After 2-3 attemps the SWT error ocurrs. 
Another issue is, that sometimes the last record is displayed in the first row and the rest of the table is empty. However, this doesn't have that high priority for us at the moment.

Comment 12 Stefan Röck 2008-06-09 02:41:46 EDT

Created attachment 104115 [details]
Snippet #2 to reproduce the SWT error

Comment 13 Rüdiger Herrmann 2008-06-09 08:32:11 EDT

 (In reply to comment #11)
> I can reproduce the SWT error with a slightly modified snippet (see attachment).
> Steps:
> 1. Click into table
> 2. Hold down the PageDown-key until the end of list is reached (should be 314
> records only)
> 
> After 2-3 attemps the SWT error ocurrs.

In a direct conveersation with Stefan, it turned out, that the problem as described above was caused by an incompletely applied patch.

> Another issue is, that sometimes the last record is displayed in the first row
> and the rest of the table is empty. However, this doesn't have that high
> priority for us at the moment.

Comment 14 Rüdiger Herrmann 2008-06-09 08:34:30 EDT

Fixed in CVS HEAD