Comment 1 Nobody - feel free to take it 2010-06-02 15:08:03 EDT

I am getting it every time I launch a DSF/GDB session.

Comment 2 Nobody - feel free to take it 2010-06-02 15:18:53 EDT

Shouldn't enableActivity be called only from the UI thread?

Comment 3 Marc Khouzam 2010-06-02 15:22:33 EDT

In the same piece of code there was a ConcurrentModificationException reported in Bug 311233

Comment 4 Marc Khouzam 2010-06-02 15:22:57 EDT

Adding Ken who knows this stuff best.

Comment 5 Marc Khouzam 2010-06-02 15:56:21 EDT

(In reply to comment #1)
> I am getting it every time I launch a DSF/GDB session.

Where do you see the exception?  I don't see it at all.
From the way the code is written, as soon as the call works once, you won't see it again, but since you see it all the time, it means you can never enable the activity.

The same should be happening for CDI and EDC.  Can you try?

Comment 6 Marc Khouzam 2010-06-02 16:01:52 EDT

(In reply to comment #2)
> Shouldn't enableActivity be called only from the UI thread?

I think you are right.  In SWT.java we can see

case ERROR_THREAD_INVALID_ACCESS:  return "Invalid thread access"; //$NON-NLS-1$

and

/** 
 * SWT error constant indicating that an attempt was made to
 * invoke an SWT operation which can only be executed by the
 * user-interface thread from some other thread
 * (value is 22).
 */
public static final int ERROR_THREAD_INVALID_ACCESS = 22;


But shouldn't this happen to all of us all the time?

Comment 7 Nobody - feel free to take it 2010-06-02 16:49:15 EDT

(In reply to comment #5)
> Where do you see the exception?  I don't see it at all.
> From the way the code is written, as soon as the call works once, you won't see
> it again, but since you see it all the time, it means you can never enable the
> activity.
>

I mean that launching fails all the time because the call always fails. 

> The same should be happening for CDI and EDC.  Can you try?

I tried CDI and got the same error. I don't understand why it doesn't happen for you - launch() is always called from a worker thread. When was this code added?

Comment 8 John Cortell 2010-06-02 16:53:55 EDT

(In reply to comment #7)
> I tried CDI and got the same error. I don't understand why it doesn't happen
> for you - launch() is always called from a worker thread. When was this code
> added?

I just tried a dsf-gdb session and changed is false in LaunchUtils.enableActivity(), thus that logic isn't exercised for me.

Comment 9 Nobody - feel free to take it 2010-06-02 17:20:05 EDT

The original error comes from here:

Caused by: org.eclipse.swt.SWTException: Invalid thread access
	at org.eclipse.swt.SWT.error(SWT.java:4083)
	at org.eclipse.swt.SWT.error(SWT.java:3998)
	at org.eclipse.swt.SWT.error(SWT.java:3969)
	at org.eclipse.swt.widgets.Widget.error(Widget.java:468)
	at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:359)
	at org.eclipse.swt.widgets.Menu.getItems(Menu.java:842)
	at org.eclipse.jface.action.MenuManager.getMenuItems(MenuManager.java:689)
	at org.eclipse.jface.action.MenuManager.update(MenuManager.java:778)
	at org.eclipse.jface.action.MenuManager.update(MenuManager.java:678)
	at org.eclipse.ui.internal.menus.WorkbenchMenuService.updateManagers(WorkbenchMenuService.java:338)
	at org.eclipse.ui.internal.menus.WorkbenchMenuService$3.activityManagerChanged(WorkbenchMenuService.java:292)
	at org.eclipse.ui.internal.activities.AbstractActivityManager.fireActivityManagerChanged(AbstractActivityManager.java:51)
	at org.eclipse.ui.internal.activities.ProxyActivityManager$1.activityManagerChanged(ProxyActivityManager.java:50)
	at org.eclipse.ui.internal.activities.AbstractActivityManager.fireActivityManagerChanged(AbstractActivityManager.java:51)
	at org.eclipse.ui.internal.activities.MutableActivityManager.updateListeners(MutableActivityManager.java:601)
	at org.eclipse.ui.internal.activities.MutableActivityManager.setEnabledActivityIds(MutableActivityManager.java:573)
	at org.eclipse.ui.internal.activities.ws.WorkbenchActivitySupport.setEnabledActivityIds(WorkbenchActivitySupport.java:320)
	at org.eclipse.cdt.dsf.debug.ui.DsfDebugUITools.enableActivity(DsfDebugUITools.java:48)
	at org.eclipse.cdt.dsf.internal.ui.DsfUIPlugin.start(DsfUIPlugin.java:74)
	at org.eclipse.osgi.framework.internal.core.BundleContextImpl$1.run(BundleContextImpl.java:783)
	at java.security.AccessController.doPrivileged(Native Method)
	at org.eclipse.osgi.framework.internal.core.BundleContextImpl.startActivator(BundleContextImpl.java:774)
	... 30 more

Comment 10 John Cortell 2010-06-02 17:30:50 EDT

(In reply to comment #9)
> The original error comes from here:

Same thing. 'changed' is never true for me in DsfDebugUITools.enableActivity()

Somehow, the set of activities in the activity manager is different for you (is missing elements). I'll see if I can track down where the dsfgdb and cdigdb ones are getting registered for me.

Comment 11 Nobody - feel free to take it 2010-06-02 17:38:13 EDT

(In reply to comment #10)
> (In reply to comment #9)
> > The original error comes from here:
> 
> Same thing. 'changed' is never true for me in DsfDebugUITools.enableActivity()
> 
> Somehow, the set of activities in the activity manager is different for you (is
> missing elements). I'll see if I can track down where the dsfgdb and cdigdb
> ones are getting registered for me.

I suspect it's because I am using a new workspace and these activities are not registered.

Comment 12 John Cortell 2010-06-02 18:43:02 EDT

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > The original error comes from here:
> > 
> > Same thing. 'changed' is never true for me in DsfDebugUITools.enableActivity()
> > 
> > Somehow, the set of activities in the activity manager is different for you (is
> > missing elements). I'll see if I can track down where the dsfgdb and cdigdb
> > ones are getting registered for me.
> 
> I suspect it's because I am using a new workspace and these activities are not
> registered.

In my the first dsf-gdb launch of a brand new runtime workbench, DsfDebugUITools.enableActivity(String, boolean) gets called only once. The activity ID argument is "org.eclipse.cdt.debug.ui.cdtActivity". That activity is pre-registered in the activity manager because of 

   <defaultEnablement id="org.eclipse.cdt.debug.ui.cdtActivity" />

in org.eclipse.cdt's plugin.xml. 

If I remove this line from the plugin.xml, create a brand new runtime workbench, and do a dsf-gdb launch, then indeed the 'changed' flag is true and 

   workbenchActivitySupport.setEnabledActivityIds(enabledActivityIds);

is called as it is in your backtrace, but the line 338 in WorkbenchMenuService.java is never executed. That is the line that will throw an exception if executed on a non-GUI thread. Some difference in our environment I assume.

Looking at the bigger picture, I don't see anything that states that WorkbenchActivitySupport.setEnabledActivityIds() needs to be called on the GUI thread. So, if that's not a requirement, it seems to me that *it* needs to ensure that any potential GUI sub-activity is pushed off to the GUI thread, thus pointing to a bug in the platform. However, if WorkbenchActivitySupport.setEnabledActivityIds() does have an undocumented requirement that it be called only from the GUI thread, then we would need to do that. I'll open a bug with the platform and see what they say.

Comment 13 John Cortell 2010-06-02 18:57:14 EDT

I opened the following platform bug, though it's not clear to me yet whether it's a platform bug or not. 

https://bugs.eclipse.org/bugs/show_bug.cgi?id=315515

Comment 14 Nobody - feel free to take it 2010-06-02 19:12:57 EDT

(In reply to comment #12)
> Some difference in our environment I assume.
The answer is simple: I forgot to check out org.eclipse.cdt.

But the potential problem is still there. It is obvious that setEnabledActivityIds() must be called from the UI thread and we have it in the code that is always called from worker threads.
BTW, do you know why the mentioned activities are not added to the "capabilities" preference?

Comment 15 John Cortell 2010-06-02 19:21:37 EDT

(In reply to comment #14)
> (In reply to comment #12)
> > Some difference in our environment I assume.
> The answer is simple: I forgot to check out org.eclipse.cdt.
> 
> But the potential problem is still there. It is obvious that
> setEnabledActivityIds() must be called from the UI thread and we have it in the
> code that is always called from worker threads.

Indeed. I'm about to post a patch, but it's theoretically prone to deadlocks, though I think they're unlikely. I could make it not be so, but then I'd be changing the behavior of the method to be asynchronous, and I'm equally (or more) nervous about what adverse consequences that would have. So, I'm not comfortable with the patch being applied at this point in the release. 

> BTW, do you know why the mentioned activities are not added to the
> "capabilities" preference?

I think the answer lies in the comments of bug 313893.

Comment 16 John Cortell 2010-06-02 19:22:56 EDT

Created attachment 170894 [details]
Possible fix for post-Helios

Comment 17 Ken Ryall 2010-06-03 08:33:06 EDT

I've tested this a lot and never seen this exception so I had no idea this could be a problem. I think this is a safe fix for Helios, but it can be done asyncExec, the launch doesn't need to wait on this.

Comment 18 Marc Khouzam 2010-06-03 09:07:58 EDT

(In reply to comment #16)
> Created an attachment (id=170894) [details]
> Possible fix for post-Helios

+1 with the async call instead.

Comment 19 John Cortell 2010-06-03 09:39:36 EDT

Created attachment 170952 [details]
Updated, simpler patch to use asyncExec. 

In this version, I just delegate to the main thread in all cases.  Personally, I think without knowing there is a valid failure scenario (Mikhail was missing an updated key cdt plugin, so that scenario is not valid), I prefer to not commit this on this last week before code freeze. If you guys think differently, feel free to commit the patch (and take the heat if it causes a regression) :-)

Comment 20 Marc Khouzam 2010-06-03 11:41:06 EDT

(In reply to comment #19)
> Created an attachment (id=170952) [details]
> Updated, simpler patch to use asyncExec. 
> 
> In this version, I just delegate to the main thread in all cases.  Personally,
> I think without knowing there is a valid failure scenario (Mikhail was missing
> an updated key cdt plugin, so that scenario is not valid), I prefer to not
> commit this on this last week before code freeze. If you guys think
> differently, feel free to commit the patch (and take the heat if it causes a
> regression) :-)

I can't think of a way to reproduce it, although Pawel did see it once.  I was worried that if some scenario came up on some setup, we didn't have a workaround so it could compromise our launches.  However, I think I found a workaround.

If we are really stuck, we can tell a user to manually edit

<workspace>/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.ui.workbench.prefs

to add the lines
UIActivities.org.eclipse.cdt.debug.cdigdbActivity=true
UIActivities.org.eclipse.cdt.debug.dsfgdbActivity=true

which will enable the activities without using the CDT code that is giving the problem.

So, I personally agree with postponing this fix until after Helios.

Comment 21 Marc Khouzam 2010-06-03 11:49:06 EDT

John, based on Bug 315515 comment #5, do you think we should make the entire code of enableActivity() be part of the asynExec call, so as to include workbenchActivitySupport.getActivityManager() which should technically be called on the UI-thread as well?

Comment 22 Marc Khouzam 2010-06-03 11:51:01 EDT

Ken, in org.eclipse.cdt/plugin.xml, there is the org.eclipse.cdt.dsf.ui.dsfActivity defined, but it is never used.  My guess is that you decided to make the DSF preference page dependent on the cdtActivity.

Should we, after Helios, remove org.eclipse.cdt.dsf.ui.dsfActivity altogether?

Comment 23 Nobody - feel free to take it 2010-06-03 11:53:09 EDT

(In reply to comment #20)
> I can't think of a way to reproduce it, although Pawel did see it once.  I was
> worried that if some scenario came up on some setup, we didn't have a
> workaround so it could compromise our launches.  However, I think I found a
> workaround.
> 
> If we are really stuck, we can tell a user to manually edit
> 
> <workspace>/.metadata/.plugins/org.eclipse.core.runtime/.settings/org.eclipse.ui.workbench.prefs
> 
> to add the lines
> UIActivities.org.eclipse.cdt.debug.cdigdbActivity=true
> UIActivities.org.eclipse.cdt.debug.dsfgdbActivity=true
> 
> which will enable the activities without using the CDT code that is giving the
> problem.
> 
A possible scenario is an external plug-in that disables one of these
activities programmatically which is a legitimate operation. In this case if a
user decides to launch a DSF or CDI session he or she would get this error and the suggested workaround wouldn't help.

Comment 24 John Cortell 2010-06-03 12:01:46 EDT

(In reply to comment #21)
> John, based on Bug 315515 comment #5, do you think we should make the entire
> code of enableActivity() be part of the asynExec call, so as to include
> workbenchActivitySupport.getActivityManager() which should technically be
> called on the UI-thread as well?

Yeah. I didn't catch that. We know that getActivityManageris safe on the non-gui thread having looked at the implementation, but we shouldn't be banking on it.

Comment 25 Ken Ryall 2010-06-03 13:18:13 EDT

Created attachment 170982 [details]
fix

All access to activity manager is now on asyncExec.

Comment 26 Ken Ryall 2010-06-03 13:19:56 EDT

Fixed for CDT 7.0.

Comment 27 CDT Genie 2010-06-03 13:38:59 EDT

*** cdt cvs genie on behalf of kryall ***
 Bug 315461 -  [launch] Invalid thread access exception during launch

[*] DsfDebugUITools.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/DsfDebugUITools.java?root=Tools_Project&r1=1.4&r2=1.5

[*] LaunchUtils.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.launch/src/org/eclipse/cdt/launch/LaunchUtils.java?root=Tools_Project&r1=1.8&r2=1.9