Comment 1 Hitesh 2008-12-02 02:58:31 EST

if (generator != null) {
	if (generator instanceof IMarkerResolutionGenerator2) {
		if (((IMarkerResolutionGenerator2) generator)
			.hasResolutions(marker)) {
				return true;
		}
	} else {
	IMarkerResolution[] resolutions = generator
						.getResolutions(marker);
		if (resolutions.length > 0) {
			// there is at least one resolution
				return true;
		}
	}
}

Although, looking at the method raises suspicions on the generator returning a null for resolutions rather than an empty array (as the JavaDoc says) , this would need more info and steps to reproduce.

If the particular generator is actually returning null, then that needs to be fixed.

Comment 2 Hitesh 2009-01-16 02:28:20 EST

The method is clean and sound enough against NPEs. I am closing this. Please feel free to reopen if you have further information on the bug.

Comment 3 Tim deBoer 2009-01-16 09:51:19 EST

The previous comment does not make any sense. The method is not sound enough against NPEs, as evidenced by the amount of NPEs that are occurring in the field.

I agree it's likely a generator not following the spec that is causing the failure, but it's causing a failure in *this* method, with no tracing to determine what else might be at fault. This method needs to be robust enough to handle the problem, either by adding a simple "resolutions != null", or code to catch and log/trace which generator is at fault.

Comment 4 Hitesh 2009-01-16 11:17:08 EST

(In reply to comment #3)

I am not convinced to adding a null check there. When a method is expected (spec'd) not to return a null, why should the caller check for it. 
If I were to follow that philosophy as a way of making code robust, I would have to be checking for nulls and logging NPEs after almost every method call. 

I'll leave this open, but I am afraid with the limited info this will see little progress.

Comment 5 Tim deBoer 2009-01-16 15:35:47 EST

FWIW, I checked and 8 additional users of our adopter product reported this during Nov.

This method is clearly failing, so how much more info do you need? I definitely do not expect null checks on every use of a variable, but it's a common Eclipse practice (and good defensive programming technique) to be defensive when calling extension points and other external components. 

Yes, the method is spec'd to not return null. However we're trusting some other team to get it right, and if they accidentally return the wrong value this method throws an uncaught exception. Worse, it fails in such a way that this method looks like the culprit and there is no way to track down who actually caused the problem. Until we improve this, people will continue to see failures that point back to org.eclipse.ui.

Comment 6 Paul Webster 2009-01-18 13:00:18 EST

(In reply to comment #5)
> Yes, the method is spec'd to not return null. However we're trusting some other
> team to get it right, and if they accidentally return the wrong value this
> method throws an uncaught exception. Worse, it fails in such a way that this
> method looks like the culprit and there is no way to track down who actually
> caused the problem. Until we improve this, people will continue to see failures
> that point back to org.eclipse.ui.

I agree, it's worth changing the method to include a useful log:

if (resolutions==null) {
  // call the status manager with the failure, something like:
  "Failure in " + element.createExecutableExtension(ATT_CLASS) + 
  " from plugin " + element.getContributor().getName() 
  + ": getResolutions() return null"
  return false;
}

PW

Comment 7 Eric Moffatt 2009-01-18 13:12:56 EST

Paul, shouldn't that be "returns null" or "returned null" ?

I might say "must not return null" since it more clearly says that someone is breaking the API contract...

Comment 8 Paul Webster 2009-01-18 13:14:54 EST

(In reply to comment #7)
> Paul, shouldn't that be "returns null" or "returned null" ?
> 
> I might say "must not return null" since it more clearly says that someone is
> breaking the API contract...

The part of interest to me is that you can print out the offending class and the bundle it came from.  But I didn't want to suggest "Some clown returned null ..." :-) 

PW

Comment 9 Hitesh 2009-01-19 07:28:38 EST

(In reply to comment #6 and comment #7)

Note that this is not the only place where we call IMarkerResolutionGenerator, and also not the only method we call that might return null. The null checking and logging would have to be added in all the other places as well.

This seems to suggest that null-checking and logging be done every time we call a 'non-null-returning' method from an extension point API (?) - if so, is this really that common a practice :)

Comment 10 Paul Webster 2009-01-19 07:37:00 EST

(In reply to comment #9)
> (In reply to comment #6 and comment #7)
> 
> Note that this is not the only place where we call IMarkerResolutionGenerator,
> and also not the only method we call that might return null. The null checking
> and logging would have to be added in all the other places as well.

The NPE protection is actually a side-effect of adding logging to track down the culprit ... :-)

PW

Comment 11 Hitesh 2009-01-19 08:09:02 EST

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #6 and comment #7)
> > 
> > Note that this is not the only place where we call IMarkerResolutionGenerator,
> > and also not the only method we call that might return null. The null checking
> > and logging would have to be added in all the other places as well.
> 
> The NPE protection is actually a side-effect of adding logging to track down
> the culprit ... :-)
> 
> PW
> 

We could have many culprits, in many different places ,striking in many different ways, and at different times :-) Do we chase all of them ;-)

What if another implementation returned a null for a different method ( also spec'd non-null-return ) at a different place, in a different way...

I'll post a patch if this is absolutely needed.

PS: This looks like a bug that should be discovered during development rather than being caught later.

Comment 12 Hitesh 2009-01-20 05:19:11 EST

Created attachment 123058 [details]
Check Null

Comment 13 Eric Moffatt 2009-01-20 16:06:57 EST

Committed in >20090120. Applied the patch...Thanks Hitesh!

Comment 14 Hitesh 2009-05-01 03:52:05 EDT

Verified in I20090429-0800.