Comment 1 Thomas Wolf 2016-03-20 19:47:15 EDT

I also run into this in Eclipse Mars.2 (4.5.2).

Stack trace:

java.lang.NullPointerException
	at org.eclipse.ui.internal.progress.StatusAdapterHelper.getStatusAdapter(StatusAdapterHelper.java:65)
	at org.eclipse.ui.internal.progress.ProgressAnimationItem.doAction(ProgressAnimationItem.java:118)
	at org.eclipse.ui.internal.progress.ProgressAnimationItem$4.widgetSelected(ProgressAnimationItem.java:348)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:248)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4230)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1491)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1514)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1499)
	at org.eclipse.swt.widgets.Widget.notifyListeners(Widget.java:1299)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4072)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3698)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:1127)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:337)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1018)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:156)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:694)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:337)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:606)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:139)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:380)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:235)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:497)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:669)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:608)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1515)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1488)

A simple null test in StatusAdapterHelper.getStatusAdapter() would "solve" the problem. I have no idea if null is expected there, or if it's a symptom of some deeper bug -- but I see that ProgressAnimationItem.doAction() is prepared to deal with a null StatusAdapter being returned...

Seems to be related to using

  setProperty(IProgressConstants.NO_IMMEDIATE_ERROR_PROMPT_PROPERTY, Boolean.TRUE);

in a WorkspaceJob.

Comment 2 Eclipse Genie 2017-08-11 12:36:50 EDT

New Gerrit change created: https://git.eclipse.org/r/102986

Comment 3 Daniel Krügler 2018-09-05 13:40:21 EDT

This problem has been recently observed for Eclipse 4.7.1 as well:

!ENTRY org.eclipse.ui 4 0 2018-09-05 05:56:27.985
!MESSAGE Unhandled event loop exception
!STACK 0
java.lang.NullPointerException
	at org.eclipse.ui.internal.progress.StatusAdapterHelper.getStatusAdapter(StatusAdapterHelper.java:65)
	at org.eclipse.ui.internal.progress.ProgressAnimationItem.doAction(ProgressAnimationItem.java:120)
	at org.eclipse.ui.internal.progress.ProgressAnimationItem.lambda$2(ProgressAnimationItem.java:339)
	at org.eclipse.swt.events.SelectionListener$1.widgetSelected(SelectionListener.java:81)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:249)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:86)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4428)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1079)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:4238)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3817)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1150)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1039)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:153)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:680)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:594)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:148)
	at com.bdal.tq.control.ui.Application.start(Application.java:29)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:653)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:590)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1499)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1472)

Code inspection of Eclipse 4.8.0 reveals the same missing null check in StatusAdapterHelper#getStatusAdapter(). 

There is still an open Gerrit change suggestion. What is the reason for not making progress here?

Comment 4 Andrey Loskutov 2018-09-05 13:46:53 EDT

(In reply to Daniel Kruegler from comment #3)
> There is still an open Gerrit change suggestion. What is the reason for not
> making progress here?

As already wrote on the gerrit we would like to k ow the root cause first. Fixing NPE can hide an issue, resulting in harder to understand issues later.

Ideally one can provide steps to reproduce or at least an analysis of the NPE.

Comment 5 Daniel Krügler 2018-09-05 14:19:14 EDT

(In reply to Andrey Loskutov from comment #4)
> (In reply to Daniel Kruegler from comment #3)
> > There is still an open Gerrit change suggestion. What is the reason for not
> > making progress here?
> 
> As already wrote on the gerrit we would like to k ow the root cause first.
> Fixing NPE can hide an issue, resulting in harder to understand issues later.
> 
> Ideally one can provide steps to reproduce or at least an analysis of the
> NPE.

The last Gerrit comment is:

"The only concern I have is about how expected it can be for this StatusAdapter to be null? If this is an entirely valid and legal state, then the patch is good. However, if this is an unexpected, illegal or error-prone state, we should log this invalid value."

I have a hard time to accept that it would be invalid to query getStatusAdapter() in way that requires the result to be non-null. Here are the arguments why that seems to be completely valid:

a) Looking from inside: Any caller of getStatusAdapter() must already prepare for a null return, because the internal map field might be null
b) Also looking from inside: There exists no method that would allow a caller to test whether the result might be null otherwise. There is also no indication that this function could no be called a second time with the same argument, but in just that case the same error would occur.
b) Looking from outside: getStatusAdapter() is currently called only at one location, namely within ProgressAnimationItem#doAction(). The code here already contains the required predicted null test:

StatusAdapter statusAdapter = StatusAdapterHelper
								.getInstance().getStatusAdapter(ji);

if (statusAdapter == null)
	statusAdapter = new StatusAdapter(status);

I completely agree that it would be preferable to have a reproducer, I'm trying to find one. But given the full generic context of the code in question I'm wondering on that being such a strong requirement given the triviality of the null test here.

Comment 6 Daniel Krügler 2018-09-06 17:05:46 EDT

Created attachment 275731 [details]
RCP client demonstrating the problem

The attachment contains an RCP client that reproduces the NPE situation. It seems to be caused when the Job property is set to IProgressConstants.NO_IMMEDIATE_ERROR_PROMPT_PROPERTY and when at least two times an error is produced. Just press twice the view toolbar contribution titled "Produce Error" and you should notice two error entries in the progress view. Now click on the status bar error icon and the NPE exception will appear:

!ENTRY org.eclipse.ui 4 0 2018-09-06 22:49:04.826
!MESSAGE Unhandled event loop exception
!STACK 0
java.lang.NullPointerException
	at org.eclipse.ui.internal.progress.StatusAdapterHelper.getStatusAdapter(StatusAdapterHelper.java:65)
	at org.eclipse.ui.internal.progress.ProgressAnimationItem.doAction(ProgressAnimationItem.java:121)
	at org.eclipse.ui.internal.progress.ProgressAnimationItem.lambda$2(ProgressAnimationItem.java:340)
	at org.eclipse.swt.events.SelectionListener$1.widgetSelected(SelectionListener.java:81)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:249)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:86)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:4118)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1052)
	at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3931)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3534)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1170)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1059)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:153)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:667)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:597)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:148)
	at reproducer438692.Application.start(Application.java:18)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:656)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:592)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1498)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1471)

The appearance of the error can be programmatically controlled within the command handler ProvokeErrorHandler, just set the local variable reproduce_438692 to false instead. Interestingly, albeit the IProgressConstants.KEEP_PROPERTY is set to true, the overall status entry machinery only keeps one of them. That's why the error only is reproducible when IProgressConstants.NO_IMMEDIATE_ERROR_PROMPT_PROPERTY is set, because now we have two entries and when clicking on the icon we enter ProgressAnimationItem#doAction() where jobTreeElements has more than one entry. The loop hits therefore twice the line

StatusAdapter statusAdapter = StatusAdapterHelper
								.getInstance().getStatusAdapter(ji);

and the second attempt causes the exception. Feel free to argue that there needs to be applied a different or additional fix.

Comment 7 Daniel Krügler 2018-09-06 17:08:52 EDT

Created attachment 275732 [details]
Screenshot showing the two error states in the Progress view

Comment 8 Daniel Krügler 2018-09-16 09:30:12 EDT

Given that I provided now a reproducer, what do we need to do to increase momentum on this issue? 

My suggestion at this point is to fix the concrete cause of the NPE in StatusAdapterHelper and once further issues are identified, these can still be handled by separate issues. For us the current situation is really bad, since the support of IProgressConstants.NO_IMMEDIATE_ERROR_PROMPT_PROPERTY is very important for our customers. 

The existing gerrit proposal is exactly what I would also propose as a fix, so I don't want to provide a new gerrit with exactly the same resolution as the existing one, unless someone explains to me that this would still be helpful.

Please give further advice.

Comment 9 Andrey Loskutov 2018-09-22 12:50:28 EDT

(In reply to Daniel Kruegler from comment #8)
> Given that I provided now a reproducer, what do we need to do to increase
> momentum on this issue? 

Given that we have a reproducer now (MANY thanks Daniel!), we can identify the root cause.

On the first loop run in ProgressAnimationItem.doAction(), the StatusAdapterHelper map containing job infos is cleared (even if there are still items to proceed) by the listener created in ProgressManager.createNotificationListener(). The NPE is coming from the bug 299385 and the clear() was added before in the bug 220557, so I would say this bug is a regression from bug 299385.

> The existing gerrit proposal is exactly what I would also propose as a fix,

Not quite, see below

> so I don't want to provide a new gerrit with exactly the same resolution as
> the existing one, unless someone explains to me that this would still be
> helpful.
> 
> Please give further advice.

Now we know the root cause we can remove the clear() call *and* add NPE check.

Interestingly, if we only add NPE check, the dialog shown on clicking the progress item will be buggy - it will contain both job names and error messages in the *same* list, not only job names as expected. Just create few entries and open the dialog to see it.

I'm still in process of thinking if ProgressManager.createNotificationListener() is still needed after the bug 299385 was removing the items, and will a propose a patch soon.

Comment 10 Eclipse Genie 2018-09-23 03:43:52 EDT

New Gerrit change created: https://git.eclipse.org/r/129838

Comment 11 Andrey Loskutov 2018-09-23 03:59:16 EDT

(In reply to Andrey Loskutov from comment #9)
> I'm still in process of thinking if
> ProgressManager.createNotificationListener() is still needed after the bug
> 299385 was removing the items, and will a propose a patch soon.

(In reply to Eclipse Genie from comment #10)
> New Gerrit change created: https://git.eclipse.org/r/129838

This is the patch I'm thinking on. 

The question I'm struggling is: ProgressManager.createNotificationListener() code *seem* to make sense originally, but if we let it be, it breaks the status dialog (which shows then broken elements). So it is safe to remove or not? Are the cases where it is still needed?

BTW, as a side effect of debugging this bug I've found bug 539348 and bug 539343.

Comment 12 Eclipse Genie 2018-10-03 04:30:43 EDT

Gerrit change https://git.eclipse.org/r/129838 was merged to [master].
Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=27d2a0ead515295171a816f0e587ba2e6f1a27a4