Bug 350113 - org.eclipse.jface.text.Position assertion due to race condition kills reconciler
Summary: org.eclipse.jface.text.Position assertion due to race condition kills reconciler
Status: NEW
Alias: None
Product: DLTK
Classification: Technology
Component: Common (show other bugs)
Version: 2.0   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: dltk.common-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-22 22:42 EDT by Carl Knight CLA
Modified: 2011-06-23 01:02 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Carl Knight CLA 2011-06-22 22:42:43 EDT
First the race condition.

We have a long file (>8000 lines) with errors galore (>2000). If I write some duff code at the end of it, or alternatively there are errors at it's end, and I perform an edit to first increase it's length and immediately edit it to reduce the length of the file fast enough, the reporting of errors for the file at it's greater length has not completed before I've reduced the length of the file. If any of those now stale errors is beyond the new length of the file this causes the assert in org.eclipse.jface.text.Position.Position(int offset, int length) on length to fail. This throws an exception and bypasses other code that looks like it is intended to handle a bad location and continue and the final outcome is the run() method of the reconciler is exited due to the thrown exception. At that point, all reconciliation for that file ceases. You can still edit it, but no building/validating occurs.

Looking at the method org.eclipse.dltk.internal.ui.editor.SourceModuleDocumentProvider.createPositionFromProblem(IProblem problem) here is what is happening:

The now stale problem's start position is > the document length (remember it now reflects the old document length because of the race condition). int length is the problem's end minus the problem's start position. Playing with the following values (purely by way of example given the race condition as described above):

length = 6.
start = 8000
end = 8006
documentLength = 7990

then in createPositionFromProblem we come to this code...

if(start + length > documentLength){
	length = documentLength - start;
}
return new Position(start, length);

start + length = 8006 which is > document length (7990). Therefore the new calculated length (length = documentLength - start) = 7990 - 8000 = -10.

The values 8000 and -10 are passed to the Position constructor which asserts on 
Assert.isTrue(length >= 0);

Note, isTrue(...) is a method in Assert.java and not a genuine java language assert.

Everything else that follows is just the callstack unwinding because of the thrown exception.

Interestingly, the code in org.eclipse.dltk.internal.ui.editor.SourceModuleDocumentProvider$SourceModuleAnnotationModel.reportProblems(SourceModuleDocumentProvider) seems designed to deal with a bad location by allowing problem reporting to continue, but is thwarted by the assert in Position throwing a org.eclipse.core.runtime.AssertionFailedException exception. Note the catch(BadLocationException) and comment below:

Position position = createPositionFromProblem(problem);
if (position != null) {

	try {
		ProblemAnnotation annotation = new ProblemAnnotation(
				problem, fSourceModule);
		overlayMarkers(position, annotation);
		addAnnotation(annotation, position, false);
		fGeneratedAnnotations.add(annotation);

		temporaryProblemsChanged = true;
	} catch (BadLocationException x) {
		// ignore invalid position
	}
}

Here is the callstack...

Exception in thread "org.eclipse.dltk.internal.ui.text.ScriptReconciler" org.eclipse.core.runtime.AssertionFailedException: assertion failed: 
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:110)
	at org.eclipse.core.runtime.Assert.isTrue(Assert.java:96)
	at org.eclipse.jface.text.Position.<init>(Position.java:63)
	at org.eclipse.dltk.internal.ui.editor.SourceModuleDocumentProvider$SourceModuleAnnotationModel.createPositionFromProblem(SourceModuleDocumentProvider.java:615)
	at org.eclipse.dltk.internal.ui.editor.SourceModuleDocumentProvider$SourceModuleAnnotationModel.reportProblems(SourceModuleDocumentProvider.java:748)
	at org.eclipse.dltk.internal.ui.editor.SourceModuleDocumentProvider$SourceModuleAnnotationModel.internalEndReporting(SourceModuleDocumentProvider.java:711)
	at org.eclipse.dltk.internal.ui.editor.SourceModuleDocumentProvider$SourceModuleAnnotationModel.endReportingSequence(SourceModuleDocumentProvider.java:699)
	at org.eclipse.dltk.internal.ui.text.ScriptCompositeReconcilingStrategy.reconcile(ScriptCompositeReconcilingStrategy.java:76)
	at org.eclipse.jface.text.reconciler.MonoReconciler.process(MonoReconciler.java:77)
	at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:206)

I suggest a either fix position to not assert, or alternatively add a catch for the assert and continue like you do for a BadLocationException.
Comment 1 Carl Knight CLA 2011-06-22 22:55:58 EDT
Possible fix:

protected Position createPositionFromProblem(IProblem problem) {
	int start = problem.getSourceStart();
	if (start < 0)
		return null;
	int end = problem.getSourceEnd();
	if (end == 0 && start == 0) {
		return new Position(0, 0);
	}
	int length = end - start;
	if (length < 0)
		return null;
	int documentLength = fDocument.getLength();
	if(start + length > documentLength){
		length = documentLength - start;
	}
	if( start >= 0 && length >= 0 )
		return new Position(start, length);
	else
		return null;
}

i.e. Ensure you return null if either parameter to the position constructor can fail the assert. null appears to be already handled where createPositionFromProblem is called from.
Comment 2 Alex Panchenko CLA 2011-06-23 01:02:47 EDT
Hi Carl,

In 3.0 (and CVS) there is the following code:

protected Position createPositionFromProblem(IProblem problem) {
	int start = problem.getSourceStart();
	int end = problem.getSourceEnd();
	if (start <= 0 && end <= 0)
		return new Position(0);
	if (start < 0)
		return new Position(end);
	if (end < 0)
		return new Position(start);
	int length = end - start;
	if (length < 0)
		return null;
	if (fDocument != null) {
		final int documentLength = fDocument.getLength();
		if (start > documentLength)
			start = documentLength;
		if (start + length > documentLength) {
			length = documentLength - start;
		}
	}
	return new Position(start, length);
}

it shouldn't fail.

Regards,
Alex