Bugzilla – Full Text Bug Listing
| Summary: | [launch] Invalid thread access exception during launch | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Pawel Piech <pawel.1.piech> | ||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Ken Ryall <ken.ryall> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | john.cortell, ken.ryall, nobody, pawel.1.piech | ||||||||
| Version: | 6.0 | ||||||||||
| Target Milestone: | 7.0 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Pawel Piech
I am getting it every time I launch a DSF/GDB session. Shouldn't enableActivity be called only from the UI thread? In the same piece of code there was a ConcurrentModificationException reported in Bug 311233 Adding Ken who knows this stuff best. (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? (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? (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? (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. 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 (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. (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 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. 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 (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? (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. Created attachment 170894 [details]
Possible fix for post-Helios
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. (In reply to comment #16) > Created an attachment (id=170894) [details] > Possible fix for post-Helios +1 with the async call instead. 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) :-)
(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. 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? 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? (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. (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. Created attachment 170982 [details]
fix
All access to activity manager is now on asyncExec.
Fixed for CDT 7.0. *** 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 |