eclipse-platform / eclipse.platform.text

8 stars 45 forks source link

Possible InvalidThreadAccess #172

Closed mickaelistria closed 1 year ago

mickaelistria commented 1 year ago

I get this stack from time to time playing with eclipseide-jdtls:

org.eclipse.swt.SWTException: Invalid thread access
    at org.eclipse.swt.SWT.error(SWT.java:4918)
    at org.eclipse.swt.SWT.error(SWT.java:4833)
    at org.eclipse.swt.SWT.error(SWT.java:4804)
    at org.eclipse.swt.widgets.Widget.error(Widget.java:565)
    at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:481)
    at org.eclipse.swt.custom.StyledText.getLineCount(StyledText.java:3665)
    at org.eclipse.jface.text.source.AnnotationRulerColumn$1.textChanged(AnnotationRulerColumn.java:194)
    at org.eclipse.jface.text.TextViewer.updateTextListeners(TextViewer.java:2789)
    at org.eclipse.jface.text.TextViewer$VisibleDocumentListener.documentChanged(TextViewer.java:396)
    at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged2(AbstractDocument.java:748)
    at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged(AbstractDocument.java:717)
    at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged(AbstractDocument.java:701)
    at org.eclipse.jface.text.AbstractDocument.fireDocumentChanged(AbstractDocument.java:775)
    at org.eclipse.jface.text.projection.ProjectionDocument.fireDocumentChanged(ProjectionDocument.java:772)
    at org.eclipse.jface.text.projection.ProjectionDocument.masterDocumentChanged(ProjectionDocument.java:742)
    at org.eclipse.jface.text.projection.ProjectionDocumentManager.fireDocumentEvent(ProjectionDocumentManager.java:126)
    at org.eclipse.jface.text.projection.ProjectionDocumentManager.documentChanged(ProjectionDocumentManager.java:132)
    at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged2(AbstractDocument.java:748)
    at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged(AbstractDocument.java:717)
    at org.eclipse.jface.text.AbstractDocument.doFireDocumentChanged(AbstractDocument.java:701)
    at org.eclipse.jface.text.AbstractDocument.fireDocumentChanged(AbstractDocument.java:775)
    at org.eclipse.jface.text.AbstractDocument.set(AbstractDocument.java:1140)
    at org.eclipse.core.internal.filebuffers.SynchronizableDocument.set(SynchronizableDocument.java:200)
    at org.eclipse.jface.text.AbstractDocument.set(AbstractDocument.java:1123)
    at org.eclipse.core.internal.filebuffers.SynchronizableDocument.set(SynchronizableDocument.java:188)

It looks like at org.eclipse.jface.text.source.AnnotationRulerColumn$1.textChanged(AnnotationRulerColumn.java:194) would need to check whether current thread is UI Thread.

szarnekow commented 1 year ago

@mickaelistria Can you help me understand why you merge this change without a review?

mickaelistria commented 1 year ago

I needed a fix urgently and this one was good and safe enough to not delay further work that was blocked by it.

iloveeclipse commented 1 year ago

@mickaelistria : can you please add missing part of the stack that should show who is calling at org.eclipse.core.internal.filebuffers.SynchronizableDocument.set(SynchronizableDocument.java:188) ?

@szarnekow has right saying that it sounds fishy that this particular event is coming from non UI thread in this comment

iloveeclipse commented 1 year ago

Actually https://github.com/eclipse-platform/eclipse.platform.text/pull/175 should be reverted, for two reasons:

1) main one: whoever calls TextViewer.updateTextListeners(TextViewer.java:2789) is supposed to do that on UI thread, and fixing one place only makes the codebase inconsistent. 2) Beside this, the call that comes from non-UI thread can talk to already disposed widget and will fail with SWT error trying to access display of that widget, so the patch itself will cause same trouble as the original code without the patch, just not that often :-)

iloveeclipse commented 1 year ago

I've stumbled over the ruler code once again, and see that the code in question that was added for this bug can be simply commented out without any visible regression (at least I don't see this bug appearing again).

Let try this instead of introducing inconsistencies in the code.