Comment 1 Dejan Glozic 2007-10-10 07:23:03 EDT

See bug 179812 for clues - it appears that changes caused by it introduced this new problem.

Comment 2 Stefan Mücke 2007-10-12 11:28:35 EDT

This bug is caused by a concurrency problem. The problem is that the access to
the 'imageCache' is not properly synchronized. 'imageCache' is accessed
by the busy thread. When a new busy thread is started, the old one
may still be running and disposing the images in the cache. That's the problem.

The last commit introduced method clearImages(). Before these changes, the images were not disposed between calls to setBusy(...).

You can reproduce the bug as follows:

 - Add the extension markup below to some plugin and paste the code below.

 - In Eclipse, add an exception breakpoint for NPE.

 - In the text field of the form, enter quickly some characters or hold down
   a key; after a short while, an execption should be thrown.

------------------------------------------------------------------------------

The following is one of several sequences that cause an exception:

- setBusy(true) is called

- setBusy(false) is called

- the current 'busyThread' leaves its while loop and enters
  its 'finally' block in order to call clearImages()

- while the the old 'busyThead' is still executing its 'finally' block,
  setBusy(true) is called again

- a new busy thread is created
  (the 'old' busy thread may still be running at this time)

- the new busy thread enters its while loop
  and calls getImage(...); it receives an image from the cache

- now the 'old' busy thread, which is still executing its
  'finally' block, disposes the image which the new busy thread
  just fetched from the cache

- finally, the new busy thread tries to draw the image
  from the cache on screen and causes an exception

------------------------------------------------------------------------------

   <extension
         point="org.eclipse.ui.views">
      <view
            name="BusyIndicatorBug"
            class="bugs.BusyIndicatorBug"
            id="bugs.BusyIndicatorBug"/>
   </extension>

------------------------------------------------------------------------------

package bugs;

import java.util.Random;

import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.Status;
import org.eclipse.core.runtime.jobs.Job;
import org.eclipse.swt.SWT;
import org.eclipse.swt.events.ModifyEvent;
import org.eclipse.swt.events.ModifyListener;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Text;
import org.eclipse.ui.forms.widgets.Form;
import org.eclipse.ui.forms.widgets.FormToolkit;
import org.eclipse.ui.part.ViewPart;

public class BusyIndicatorBugView extends ViewPart {

	private Form form;
	private Text text;

	public BusyIndicatorBugView() {
	}

	@Override
	public void createPartControl(Composite parent) {
		FormToolkit toolkit = new FormToolkit(Display.getCurrent());
		form = toolkit.createForm(parent);
		form.setText("Header");
		toolkit.decorateFormHeading(form);
		Composite body = form.getBody();
		toolkit.paintBordersFor(body);
		body.setLayout(new GridLayout());

		text = toolkit.createText(body, "Enter text here");
		text.addModifyListener(new ModifyListener() {
			public void modifyText(ModifyEvent e) {
				startBackgroundJob();
			}
		});
		text.setLayoutData(new GridData(SWT.FILL, SWT.BEGINNING, true, false));
	}

	@Override
	public void setFocus() {
		text.setFocus();
	}

	private void startBackgroundJob() {
		Job job = new Job("Bug Job") {
			@Override
			protected IStatus run(IProgressMonitor monitor) {
				setBusy(true);
				doSomeWork();
				setBusy(false);
				return Status.OK_STATUS;
			}
			private void doSomeWork() {
				try {
					Thread.sleep(20 + new Random().nextInt(200));
				} catch (InterruptedException e) {
				}
			}
			private void setBusy(final boolean b) {
				Display.getDefault().syncExec(new Runnable() {
					public void run() {
						form.setBusy(b);
					}
				});
			}
		};
		job.schedule();
	}

}

Comment 3 Stefan Mücke 2007-11-07 13:10:31 EST

Created attachment 82347 [details]
Patch against HEAD to fix this bug

This patch fixes all problems described. The following changes were made:

- added missing synchronization for access to the image cache
- refactored the anonymous busy thread class into an internal class
- moved field 'stop' to BusyThread class

The last mentioned change is necessary because otherwise it may happen that a thread is not actually stopped, namely, if in the meantime a new busy thread is created (and thus stop is reset to false). In this case, two busy threads draw at the same time, causing some strange flickering.

The patch has been thoroughly tested for some time. No further problems were encountered.

Comment 4 Adam Archer 2007-11-07 13:16:50 EST

Providing patches for your own bugs! I like the way you operate.

Thanks Stefan, I'll test this out.

Comment 5 Chris Goldthorpe 2007-11-07 13:26:50 EST

Thanks Stefan! Adam can you review this patch?

Comment 6 Stefan Mücke 2007-11-08 09:23:36 EST

Created attachment 82441 [details]
Patch against HEAD to fix this bug (slightly updated)

I had a quick glance at the code again. Before committing these changes, perhaps some 'final' keywords should be removed. These partly existed before because of the annonymous class, and partly resulted from the (automatic) refactoring. None of them is still necessary. Some 'private' keywords can also be removed.

Moreover, I thought about moving the lines of code before 'new BusyThread(...)' directly into the BusyThread constructor; however, this does not work, since the code must run in the display thread. So these lines should not be moved.

Comment 7 Adam Archer 2007-11-30 14:06:12 EST

Patch in comment 6 tested very well.

Thanks again Stefan!

+1

Comment 8 Chris Goldthorpe 2007-11-30 15:02:41 EST

I'll commit this next week.

Comment 9 Chris Goldthorpe 2007-12-03 14:35:45 EST

Fixed in HEAD.