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

Bug 312709

Summary: [Launch] NPE in GdbLaunchDelegate
Product: [Tools] CDT Reporter: Navid Mehregani <nmehrega>
Component: cdt-debug-dsf-gdbAssignee: Project Inbox <cdt-debug-dsf-gdb-inbox>
Status: RESOLVED FIXED QA Contact: Marc Khouzam <marc.khouzam>
Severity: normal    
Priority: P3 CC: jamesblackburn+eclipse, john.cortell, marcin.swiezawski, pawel.1.piech
Version: 7.0   
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
path for NPE john.cortell: iplog-

Description Navid Mehregani CLA 2010-05-12 16:30:58 EDT 
The following NPE occurs if multiple DSF-GDB sessions are started right after one another:

Thread [Worker-23] (Suspended (exception NullPointerException))	
	CProjectDescriptionManager.setProjectDescription(IProject, ICProjectDescription, int, IProgressMonitor) line: 837	
	CProjectDescriptionManager.setProjectDescription(IProject, ICProjectDescription, boolean, IProgressMonitor) line: 804	
	CProjectDescriptionManager.setProjectDescription(IProject, ICProjectDescription) line: 799	
	CoreModel.setProjectDescription(IProject, ICProjectDescription) line: 1389	
	CDTPropertyManager.performOkForced(Object) line: 141	
	CDTPropertyManager.performOk(Object) line: 106	
	GdbLaunchDelegate(AbstractCLaunchDelegate2).setBuildConfiguration(ILaunchConfiguration, IProject) line: 299	
	GdbLaunchDelegate(AbstractCLaunchDelegate2).buildForLaunch(ILaunchConfiguration, String, IProgressMonitor) line: 214	
	LaunchConfiguration.launch(String, IProgressMonitor, boolean, boolean) line: 821	
	LaunchConfiguration.launch(String, IProgressMonitor, boolean) line: 702	
	DebugUIPlugin.buildAndLaunch(ILaunchConfiguration, String, IProgressMonitor) line: 923	
	DebugUIPlugin$8.run(IProgressMonitor) line: 1126	
	Worker.run() line: 54	


Reproduced on M7 build
Steps to reproduce: Setup a DSF-GDB debug session.  Start launching the session like a mad-man.
Comment 1 James Blackburn CLA 2010-05-12 16:45:37 EDT 
It's not good to use CDTPropertyManager in this way - this is a UI utility class only intended to be accessed from the UI thread (by the property pages).  Ideally it should assert that Display currentThread == default...

It's also a bad idea to change build configuration like this before launch. If the user has active configuration follows build config, you'll kick off the indexer.

You should be able to request a build of a specific configuration by just setting some an appropriate build argument -- whether we have an easily accessible utility class to help with this I'd have to dig...
Comment 2 John Cortell CLA 2010-05-21 18:23:05 EDT 
(In reply to comment #1)
> It's not good to use CDTPropertyManager in this way - this is a UI utility
> class only intended to be accessed from the UI thread (by the property pages). 
> Ideally it should assert that Display currentThread == default...
> 
> It's also a bad idea to change build configuration like this before launch. If
> the user has active configuration follows build config, you'll kick off the
> indexer.
> 
> You should be able to request a build of a specific configuration by just
> setting some an appropriate build argument -- whether we have an easily
> accessible utility class to help with this I'd have to dig...

I've stumbled on a nasty dilemma. If we don't change the build configuration and there are errors, then the launch aborts but the user is left in an environment that may not be set to build what we just tried to build, and that could be very confusing. Imagine that the active build configuration builds fine, but we just told him we can't launch because there's a build error. Ugh. This is turning out to be a much trickier issue than I anticipated, and I'm thinking it's time to pull the parachute on this one, considering we've just entered RC3. Let's tackle this after Helios.
Comment 3 James Blackburn CLA 2010-05-21 18:52:54 EDT 
Created attachment 169568 [details]
path for NPE

(In reply to comment #2)
> I've stumbled on a nasty dilemma. If we don't change the build configuration
> and there are errors, then the launch aborts but the user is left in an
> environment that may not be set to build what we just tried to build, and that
> could be very confusing. Imagine that the active build configuration builds

Ah, ok. I see that the launch delegate leaves the configuration as built if there were errors.

I still think PropertyManager shouldn't be used, even for Helios.  Something like that attached patch (untested -- just typed in as its late here), would be safer.

There's identical code in AbstractCLaunchDelegate, is there a better way than having all this duplication?
Comment 4 John Cortell CLA 2010-05-24 17:35:09 EDT 
(In reply to comment #3)
> Created an attachment (id=169568) [details]
> path for NPE
> 
> (In reply to comment #2)
> > I've stumbled on a nasty dilemma. If we don't change the build configuration
> > and there are errors, then the launch aborts but the user is left in an
> > environment that may not be set to build what we just tried to build, and that
> > could be very confusing. Imagine that the active build configuration builds
> 
> Ah, ok. I see that the launch delegate leaves the configuration as built if
> there were errors.

I decided to mitigate the issue by putting up a more informative error dialog for the build errors in this case, which tells the user that the errors were in a not-active build configuration. I did this as part of bug # 309126.

> 
> I still think PropertyManager shouldn't be used, even for Helios.  Something
> like that attached patch (untested -- just typed in as its late here), would be
> safer.
> 
> There's identical code in AbstractCLaunchDelegate, is there a better way than
> having all this duplication?

Refer to bug # 285907.
Comment 5 Marc Khouzam CLA 2010-05-25 08:54:10 EDT 
I'm confused as to where we are with this bug.  Do we still have a risk of an NPE?
Comment 6 James Blackburn CLA 2010-05-25 08:59:11 EDT 
(In reply to comment #5)
> I'm confused as to where we are with this bug.  Do we still have a risk of an
> NPE?

John's patch on bug 309126 will fix this.
Comment 7 Marc Khouzam CLA 2010-05-31 09:27:32 EDT 
John, is this fixed?
Comment 8 John Cortell CLA 2010-06-01 10:17:22 EDT 
Fixed by 309126