Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 315515

Summary:	[ActivityMgmt] IWorkbenchActivitySupport.setEnabledActivityIds(Set) must be called on the GUI thread
Product:	[Eclipse Project] Platform	Reporter:	John Cortell <john.cortell>
Component:	IDE	Assignee:	Platform UI Triaged <platform-ui-triaged>
Status:	CLOSED INVALID	QA Contact:	Paul Webster <pwebster>
Severity:	normal
Priority:	P3	CC:	marc.khouzam, nobody
Version:	3.6
Target Milestone:	---
Hardware:	PC
OS:	Windows XP
Whiteboard:

Description John Cortell

2010-06-02 18:54:15 EDT

We hace some code in CDT that calls 

   IWorkbenchActivitySupport.setEnabledActivityIds(Set)

on a non-GUI thread. This ends up failing on some machines because it ultimately exercises jFace code on the caller's thread. The callstack can be seen here:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=315461#c9

It seems to me that either 

(a) the interface method should declare that it has to be called on the GUI thread (unless this is somehow implied?), or
(b) the implementation should guarantee that any GUI activity is delegated to the main thread

Comment 1 John Cortell

2010-06-03 09:00:52 EDT

BTW, even the workbench itself exercises this method on a non-GUI thread. On startup...

Thread [Thread-1] (Suspended (breakpoint at line 320 in org.eclipse.ui.internal.activities.ws.WorkbenchActivitySupport))	
	org.eclipse.ui.internal.activities.ws.WorkbenchActivitySupport.setEnabledActivityIds(java.util.Set) line: 320	
	org.eclipse.ui.internal.ActivityPersistanceHelper.loadEnabledStates(java.util.Set, java.util.Set) line: 258	
	org.eclipse.ui.internal.ActivityPersistanceHelper.loadEnabledStates() line: 206	
	org.eclipse.ui.internal.ActivityPersistanceHelper.<init>() line: 154	
	org.eclipse.ui.internal.ActivityPersistanceHelper.getInstance() line: 122	
	org.eclipse.ui.internal.Workbench.init() line: 1509	
	org.eclipse.ui.internal.Workbench.access$33(org.eclipse.ui.internal.Workbench) line: 1471	
	org.eclipse.ui.internal.Workbench$61.run() line: 2525

Comment 2 Paul Webster

2010-06-03 09:11:47 EDT

(In reply to comment #1)
> BTW, even the workbench itself exercises this method on a non-GUI thread. On
> startup...

This method should only be called on the UI thread.  The Platform UI policy is that all support and services (commands, handlers, menu contributions, evaluations, etc) should be called on the UI thread unless the javadoc specifies otherwise.

The init case is not interesting as it is done early in the workbench initialization, before any windows have even been created.

PW

Comment 3 John Cortell

2010-06-03 09:25:05 EDT

OK. Thanks for the clarification. It looks like we have some work to do in CDT since it looks like we're not honoring that policy (in more than just this case).

Comment 4 Marc Khouzam

2010-06-03 09:26:22 EDT

(In reply to comment #2)
> (In reply to comment #1)
> > BTW, even the workbench itself exercises this method on a non-GUI thread. On
> > startup...
> 
> This method should only be called on the UI thread.  The Platform UI policy is
> that all support and services (commands, handlers, menu contributions,
> evaluations, etc) should be called on the UI thread unless the javadoc
> specifies otherwise.

So all calls to IWorkbenchActivitySupport should be on the UI thread?  Even something seemingly safe like IWorkbenchActivitySupport.getActivityManager()?

Comment 5 Paul Webster

2010-06-03 09:49:29 EDT

(In reply to comment #4)
> So all calls to IWorkbenchActivitySupport should be on the UI thread?  Even
> something seemingly safe like IWorkbenchActivitySupport.getActivityManager()?

The short answer is yes.  The longer answer is: we haven't cleared that method to run off of the UI thread, and since Activities are a UI construct then dealing with them might have UI repercussions.

In this case, getting the manager just returns the already created manager.  If it did some kind of lazy initialization instead that might catch you out (sometimes even pre-loading JFace colours will cause exceptions).  But most of the methods on that manager are UI affecting.  Even getActivity("id") will create and update an activity if one doesn't exist, which could cause updates in UI affecting objects.

If there is a need for a specific method that you think is harmless to be called off the UI thread (say commandService.getCommands() which returns all commands in the system) then you can open bug and we can assess the call.

But calls into the Platform UI framework are mostly UI affecting and must be done on the UI thread.

PW

Comment 6 John Cortell

2010-06-03 10:00:09 EDT

(In reply to comment #5)
> (In reply to comment #4)
> > So all calls to IWorkbenchActivitySupport should be on the UI thread?  Even
> > something seemingly safe like IWorkbenchActivitySupport.getActivityManager()?
> 
> The short answer is yes.  
> [snip]

That makes sense to me. I guess another way to look at it is if you're querying the activity manager, it's because you intend to use it, and using it will require the main thread, so requiring the query to be on the main thread doesn't seem like an additional burden.