Community
Participate
Working Groups
Build Identifier: I20090611-1540 The function CConfigBasedDescriptorManager::doHandleEvent() from the org.eclipse.cdt.internal.core package throws an NullPointerException if its event parameter has a null project property. It should be coded more defensively to handle this case. Similarly, BuildStateManager::removeProjectInfo() from org.eclipse.cdt.managedbuilder.internal.buildmodel should simply return if its project parameter is null. Reproducible: Always
Created attachment 157815 [details] Proposed patch for BuildStateManager.java
Created attachment 157816 [details] Proposed patch for CConfigBasedDescriptorManager.java
I touched this code last, so I'll bite... Quick question: why? How are you generating ProjectDescriptionEvents without a project? In general user/ISV code should not be generating these events, instead they're always generated by the configuration model as the result of some internal change. AFAICS all the sites that use this pass in a cfgdesc as required. Can you post a backtrace of where you're seeing this go wrong?
We are not generating these events directly. The problem happens in application-specific code that creates several CDT projects and sets up their settings and dependencies. When we try to call Configuration::createFolderInfo() function, it throws a NullPointerException that we were able to trace back to two internal CDT functions. The proposed patches simply make these function more robust so that they would not throw the exception. It is unclear why they are being called with null projects in parameters. I attach a backtrace of the problem.
Created attachment 157833 [details] The error log fragment with a call stack
Ok, thanks. What's interesting in your backtrace is that you're calling setProjectDescription from within your IManagedOptionValueHandler. I don't think this is a legal thing to do - especially as the value handler is called back when the user changes option in the UI without pressing apply... Furthermore I think that the Project should _never_ be null during these events. Adding null checks may maks the problem, but in my opinion is not the correct fix. Do you see the problem if you code doesn't do this / have you ever made CDT itself do this?
As per comment 6, I think the way you're using the API is incorrect. Please reopen if you can reproduce the issue in CDT proper, or you think you've spotted an underlying issue. AFAICS adding null checks is not a valid fix in this case.
Sorry it took me a while to get back to this. We removed the setProjectDescription() call from the our IManagedOptionValueHandler instance, and the createFolderInfo() call no longer throws NullPointerExceptions. I see three distinct problems here however, each worthy of attention. The first problem is the rules when ISVs should and should not call setProjectDescription() are AFAIK not documented anywhere. We discover that this call is required to make option changes persistent, but have no way of knowing that this is not the case inside IManagedOptionValueHandler callbacks. Adding some usage guidelines to the JavaDoc comment of setProjectDescription() would be very helpful. The second problem is that a setProjectDescription() call with valid parameters can cause CDT internals to generate ProjectDescriptionEvents with null project properties. This is a "CDT itself" problem, and as we observed it can be difficult to isolate and to deal with. The third problem is two functions I proposed to patch have insufficient or nonexistent parameter validation. I agree that if their parameters by design should always point to valid projects, simply to return in case if a null project would not be a good fix. To let Java runtime throw a NullPointerException however is even worse. I think the right approach would be to do a null project check and either write to the error log or throw a meaningful exception or both.
(In reply to comment #8) > The first problem is the rules when ISVs should and should > not call setProjectDescription() are AFAIK not documented anywhere. > We discover that this call is required to make option changes persistent, > but have no way of knowing that this is not the case inside > IManagedOptionValueHandler callbacks. Adding some usage guidelines to > the JavaDoc comment of setProjectDescription() would be very helpful. Granted. The problem, to reiterate, is that it's wrong to setProjectDescription in this call-back. The call-back's javadoc clearly states: * This interface represents an option value handler in the managed build * system. It is used to enable a tool integrator to use the MBS configuration * GUI, while linking to an alternative back-end. Note that when a user presses apply / OK the UI causes a setProjectDescription to occur, so you doing similarly will lead to a race and one set of settings may not be persisted. > The second problem is that a setProjectDescription() call with valid > parameters can cause CDT internals to generate ProjectDescriptionEvents > with null project properties. This is a "CDT itself" problem, and as we > observed it can be difficult to isolate and to deal with. The API usage is wrong, that's why you got the NPE. This bug only occurs as a result of your callback, right? > The third problem is two functions I proposed to patch have insufficient > or nonexistent parameter validation. I agree that if their parameters by design > should always point to valid projects, simply to return in case if a null > project would not be a good fix. To let Java runtime throw a > NullPointerException however is even worse. I think the right approach would be > to do a null project check and either write to the error log or throw a > meaningful exception or both. I agree assertLegal(project != null) would be a better fix, but the NPE is an indication that something has gone badly wrong. This is why I closed the bug INVALID / NOT_ECLIPSE: it's an invalid use of the API. The NPE exposed a bug in the way the API was being used. I do agree we could assertLegal or some such, but silently handling the null is the wrong fix. If you want to submit a patch to sanity check the arguments I'd be happy to take a look.
