SWTException: Invalid thread access in Memory view

[jonahgraham]

This is a regression in Eclipse 4.29 compared to 4.28 - when memory view is refreshed, common after editing a cell.

This is the stacktrace:

!ENTRY org.eclipse.debug.ui 4 120 2023-09-20 16:20:57.444
!MESSAGE Error logged from Debug UI: 
!STACK 0
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.widgets.Table.setRedraw(Table.java:3747)
	at org.eclipse.jface.viewers.StructuredViewer.refresh(StructuredViewer.java:1425)
	at org.eclipse.debug.internal.ui.memory.provisional.AbstractAsyncTableRendering.refresh(AbstractAsyncTableRendering.java:2107)
	at org.eclipse.debug.internal.ui.views.memory.renderings.AsyncTableRenderingUpdatePolicy.handleMemoryBlockChanged(AsyncTableRenderingUpdatePolicy.java:145)
	at org.eclipse.debug.internal.ui.views.memory.renderings.AsyncTableRenderingUpdatePolicy.modelChanged(AsyncTableRenderingUpdatePolicy.java:76)
	at org.eclipse.debug.internal.ui.viewers.provisional.AbstractModelProxy$1.run(AbstractModelProxy.java:92)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.debug.internal.ui.viewers.provisional.AbstractModelProxy.fireModelChanged(AbstractModelProxy.java:96)
	at org.eclipse.debug.internal.ui.viewers.update.MemoryBlockProxy$1.handleChange(MemoryBlockProxy.java:54)
	at org.eclipse.debug.internal.ui.viewers.update.EventHandlerModelProxy.dispatchChange(EventHandlerModelProxy.java:237)
	at org.eclipse.debug.internal.ui.viewers.update.EventHandlerModelProxy.handleDebugEvents(EventHandlerModelProxy.java:137)
	at org.eclipse.debug.core.DebugPlugin$EventNotifier.run(DebugPlugin.java:1225)
	at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:45)
	at org.eclipse.debug.core.DebugPlugin$EventNotifier.dispatch(DebugPlugin.java:1259)
	at org.eclipse.debug.core.DebugPlugin$EventDispatchJob.run(DebugPlugin.java:491)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)

This can easily be replicated with CDT by opening memory view, adding a memory monitor, opening a hex rendering and editing a cell.

This can also be replicated using the platform debug example PDA launch type in a dev environment

1. Add the debug example bundles to your workspace (all of the bundles with examples in their name in the repo)
2. Launch a runtime workbench
3. Create a simple project which has the PDA samples
4. Create a PDA Application debug launch configuration for counter.pda
5. Launch in debug mode
6. Press pause
7. Open Memory View
8. Press Add (next to Monitors label)
9. Fill in 100 for address and 100 for number of bytes
10. (maybe needed, seems like a different bug) change to a different view in same tab group and back again to display the rendering
11. click in first cell and type a new number
12. press Enter
13. Observe error log has errors like above

Here is a screen recording as I am sure that some people may not know about this:

Peek.2023-09-20.16-32.mp4

[jonahgraham]

The Memory view uses a custom viewer org.eclipse.debug.internal.ui.viewers.AsynchronousViewer that operates quite differently than most viewers as its methods can be called from non-UI thread. e.g refresh can be called from a worker thread.

The issue is that eclipse-platform/eclipse.platform.ui@3b9c02c changed refresh method to access the control which is called on non-UI thread.

[jonahgraham]

For completeness - if the UI was working the edited cell would be redrawn to look like this:

[jonahgraham]

My first instinct is to say that AsynchronousViewer is doing something quite odd by being callable from non-UI thread. Is there anyone else out there that relies on such behaviour and is it ok for anyone to rely on such behaviour?

There are two fixes I can think of:

1. Change the async viewer to restore the old refresh code:

diff --git a/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/AsynchronousViewer.java b/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/AsynchronousViewer.java
index b287456..a15ff50 100644
--- a/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/AsynchronousViewer.java
+++ b/debug/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/viewers/AsynchronousViewer.java
@@ -415,6 +415,11 @@
 	}
 
 	@Override
+	public void refresh() {
+		refresh(getRoot());
+	}
+
+	@Override
 	protected void internalRefresh(Object element) {
 		// get the nodes in the model
 		AsynchronousModel model = getModel();

or do a little more checking like @Bananeweizen did to fix similar issue eclipse-platform/eclipse.platform.ui#934 and add a thread check before trying to turn off redraw:

diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/StructuredViewer.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/StructuredViewer.java
index e2f50da..b1d1d17 100644
--- a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/StructuredViewer.java
+++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/StructuredViewer.java
@@ -1421,13 +1421,13 @@
 	@Override
 	public void refresh() {
 		Control control = getControl();
-		if (control != null) {
+		if (control != null && control.getDisplay().getThread() == Thread.currentThread()) {
 			control.setRedraw(false);
 		}
 		try {
 			refresh(getRoot());
 		} finally {
-			if (control != null) {
+			if (control != null && control.getDisplay().getThread() == Thread.currentThread()) {
 				control.setRedraw(true);
 			}
 		}

[jonahgraham]

@Bananeweizen or @vogella as you both were involved with the initial change I was wondering if either of you had thoughts on this? Or anyone else wants to weigh in?

[iloveeclipse]

or do a little more checking like @Bananeweizen did to fix similar issue eclipse-platform/eclipse.platform.ui#934 and add a thread check before trying to turn off redraw:

That is different here. I wouldn't expect JFace viewer's code being called from non UI thread, that is guaranteed to fail sooner or later.

1. Change the async viewer to restore the old refresh code

This is better, but I wonder if the code that calls refresh in the memory view should run via asyncExec() if executed from non UI thread. May be somewhere at AbstractModelProxy.fireModelChanged().

[jonahgraham]

The entirety of that class is designed to be called asynchronously. Rewriting all its call sites seems way too much work and would largely undo the idea of the class anyway.

[laeubi]

I just wanted to mention that there is Display#execute that handles all the details of sync vs ansyc call depending on thread context.

Beside that making the Viewer async is a bit bad idea as @iloveeclipse already mentioned, JFace is not threadsafe and as it is interacting with the SWT Thread is is mostly a good idea to only permutate anything in the SWT (UI) thread.

If you like to perform certain actions in the background (e.g. loading the model), the preferred idom would be something similar to

CompletableFuture.supplyAsync(()->{
    //do the heavy work here
}).thenAcceptAsync(result->viewer::setInput, viewer.getControl().getDisplay());

If you like you can even use the new BusyIndicator#compute / BusyIndicator#execute

[jonahgraham]

It was an interesting design decision to reuse the jface Viewer class to be asynchronous. As I look into this code further I agree that having Viewer try to maintain some undocumented feature of not touching UI thread is not a great idea.

Fortunately the async viewer class hierarchy is internal to platform debug and if someone wants to refactor it one day that would be great.

For now I am going to move this issue to platform as that is where platform debug ui is today and supply a PR that fixes this issues consistently with how the rest of the async viewers are written.

[Kummallinen]

A comment on the severity of this bug, it makes 2023-09 essentially unstable for anyone who's workflow uses the memory view. Performing any edit or a memory import currently requires a manual refresh before the edited memory is shown. It is also silent unless you have the error log open so users may not realise it is a rendering issue.

I am aware there is no process for building a patch release for users. But could the fix be backported to the maintenance branch? This would make it easier for those to build their own products on top of Eclipse to pick up a fix. Especially as there are other important fixes in SimRel 2023-09 & Platform 4.29 that make updating more important than usual.

[laeubi]

But could the fix be backported to the maintenance branch?

Everyone can create PRs targeting any branch.

This would make it easier for those to build their own products on top of Eclipse to pick up a fix

Of course one can also simply reference the I-Build repository until it is fully released.

[jonahgraham]

But could the fix be backported to the maintenance branch?

@Kummallinen I have started backporting this to 4.29 branch in #713