Community
Participate
Working Groups
I got this error and trace when I tried to create a new bug. I may have put in an incorrect CC. java.lang.NullPointerException at org.eclipse.jface.dialogs.IconAndMessageDialog.getSWTImage(IconAndMessageDialog.java:265) at org.eclipse.jface.dialogs.IconAndMessageDialog.getErrorImage(IconAndMessageDialog.java:212) at org.eclipse.jface.dialogs.MessageDialog.<init>(MessageDialog.java:141) at org.eclipse.jface.dialogs.MessageDialog.openError(MessageDialog.java:310) at org.eclipse.mylar.bugzilla.ui.tasklist.BugzillaTask$OpenBugTaskJob.run(BugzillaTask.java:386) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:76)
I am unable to reproduce this, and it happens from a message dialog, so we will lower the priority and see if it happens
I have the same problem then I try use MyEclipse. I am trying to add Spring capabilites to web project. java.lang.NullPointerException at org.eclipse.jface.dialogs.IconAndMessageDialog.getSWTImage(IconAndMessageDialog.java:265) at org.eclipse.jface.dialogs.IconAndMessageDialog.getQuestionImage(IconAndMessageDialog.java:242) at org.eclipse.jface.dialogs.MessageDialog.<init>(MessageDialog.java:149) at com.genuitec.eclipse.springframework.wizards.NewSpringSupportCreationWizard.installExplodedLibraryContainers(NewSpringSupportCreationWizard.java:385) at com.genuitec.eclipse.springframework.wizards.NewSpringSupportCreationWizard.installSupportLibraries(NewSpringSupportCreationWizard.java:246) at com.genuitec.eclipse.springframework.wizards.NewSpringSupportCreationWizard.finishPage(NewSpringSupportCreationWizard.java:113) at com.genuitec.eclipse.core.portability.NewElementWizard$2.run(UnknownSource) at org.eclipse.jdt.internal.core.BatchOperation.executeOperation(BatchOperation.java:39) at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:718) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1719) at org.eclipse.jdt.core.JavaCore.run(JavaCore.java:3760) at org.eclipse.jdt.internal.ui.actions.WorkbenchRunnableAdapter.run(WorkbenchRunnableAdapter.java:87) at org.eclipse.jface.operation.ModalContext$ModalContextThread.run(ModalContext.java:113)
This bug is caused by the display being null when executing the sycnExec: display.syncExec(new Runnable() { public void run() { image[0] = display.getSystemImage(imageID); } }); This issue appears to be unrelated to Mylar so moving to JFace. We get this problem by calling the following from within the execution of a wizard (similar to comment #2). Note the null parent argument, which seems like it should be fine. MessageDialog.openError(null, "Error Opening Bug", "Unable to open Bug report: " + BugzillaTask.getBugId(bugTask.getHandle()));
Moving to SWT inbox (not sure if it's the right one, but this report has had the wrong assignee for some time).
Mik, are you still seeing this problem, and if so, with what release/build of Eclipse? I need more information to reproduce this one. Coding around a null display is not the answer, because the call chain of this method expects to be able to get a valid image. So answering null when the display is null will just move the error somewhere else. We need to figure out under what conditions the shell's display (or default display) is null.
On Mylar this problem no longer surfaces, but that's because we added asyncExecs to the couple of places that triggered it. So the underlying problem may still be there. As suggested by comment#2 and comment#3, if this is still a bug you should be able to trigger it by running MessageDialog.openError(..) from within a wizard's finish page, which is where we were seeing it. I don't recall if it neede to be in performFinish() or not. Let me know if that's not enough info to reproduce and I can try to recreate the bug from CVS history.
changing prio and status to conform to plat-ui bug guidelines
I get this problem when launching Tomcat inside MyEclipse when a profiling agent is in place. I'll let the MyEclipse folks know as well. !ENTRY com.genuitec.eclipse.easie.core 1 0 2007-10-17 16:25:48.868 !MESSAGE Error starting Tomcat 5.x server !STACK 0 com.genuitec.eclipse.core.GenuitecCoreException: Error occurred during initialization of VM Could not find agent library in absolute path: /Applications/jprofiler5/bin/macos/libjprofilerti.jnilib at com.genuitec.eclipse.easie.tomcat.Tomcat.start(Tomcat.java:52) at com.genuitec.eclipse.easie.tomcat.Tomcat.start(Tomcat.java:42) at com.genuitec.eclipse.easie.core.ui.action.ServerStartAction.ă(Unknown Source) at com.genuitec.eclipse.easie.core.ui.action.ServerStartAction.basicRun(Unknown Source) at com.genuitec.eclipse.core.ui.action.LicenseValidatingAction.run(Unknown Source) at com.genuitec.eclipse.easie.core.ui.action.ServerRestartAction$1.run(Unknown Source) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55) Caused by: org.eclipse.core.runtime.CoreException: Error occurred during initialization of VM Could not find agent library in absolute path: /Applications/jprofiler5/bin/macos/libjprofilerti.jnilib at org.eclipse.jdt.launching.AbstractVMRunner.abort(AbstractVMRunner.java:47) at org.eclipse.jdt.internal.launching.StandardVMDebugger.checkErrorMessage(StandardVMDebugger.java:434) at org.eclipse.jdt.internal.launching.StandardVMDebugger.run(StandardVMDebugger.java:259) at com.genuitec.eclipse.core.JavaVMUtility.execute(JavaVMUtility.java:214) at com.genuitec.eclipse.easie.tomcat.TomcatManager.startTomcat(TomcatManager.java:56) at com.genuitec.eclipse.easie.tomcat.TomcatManager.start(TomcatManager.java:36) at com.genuitec.eclipse.easie.tomcat.Tomcat.start(Tomcat.java:49) ... 6 more !ENTRY org.eclipse.core.jobs 4 2 2007-10-17 16:25:48.871 !MESSAGE An internal error occurred during: "ServerRestartAction_server_restart". !STACK 0 java.lang.NullPointerException at org.eclipse.jface.dialogs.IconAndMessageDialog.getSWTImage(IconAndMessageDialog.java:275) at org.eclipse.jface.dialogs.IconAndMessageDialog.getErrorImage(IconAndMessageDialog.java:225) at org.eclipse.jface.dialogs.MessageDialog.<init>(MessageDialog.java:141) at com.genuitec.eclipse.easie.core.ui.action.PreferenceLinkDialog.<init>(Unknown Source) at com.genuitec.eclipse.easie.core.ui.action.ServerStartAction.ā(Unknown Source) at com.genuitec.eclipse.easie.core.ui.action.ServerStartAction.basicRun(Unknown Source) at com.genuitec.eclipse.core.ui.action.LicenseValidatingAction.run(Unknown Source) at com.genuitec.eclipse.easie.core.ui.action.ServerRestartAction$1.run(Unknown Source) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)
As per http://wiki.eclipse.org/Platform_UI/Bug_Triage_Change_2009
Created attachment 141395 [details] Fix The NPE is very difficult to spot: it looks like it is possible to use getSWTImage in non UI Thread (display.syncExec), but on the other hand Display.getCurrent is used, which returns null for non-UI. And hence NPE. I suggest using Display.getDefault instead of Display.getCurrent.
Susan, can you review and commit/give green light to commit the patch for 3.6?
Thanks for looking into this, Christopher. We definitely need to kill some of these older bugs. But I'm not so sure about this fix. The stack traces provided (comment 0, comment 2, comment 8) all involve a call to a message dialog from a non-UI thread (either a modal context that is forked or a job). In these situations, the caller should be invoking the message dialog using an asyncExec (or syncExec if appropriate). I guess this wasn't obvious to me when I first looked at this bug. If we simply cover up the NPE by using the default display, I would expect an SWT "invalid thread access" failure shortly after. (Maybe that is more informative!) The correct answer is that the client use an async (as Mik ended up doing). In looking at the code history, there was originally a bug in this area involving an NPE when a null shell was passed in (bug 61743). That bug was fixed and in doing so, the display.syncExec block was introduced. Honestly, that block doesn't make any sense to me. I agree with Christopher that the syncExec used in getSWTImage seems to "imply" that the icon retrieval methods should be non-UI thread safe. However, the spec for the public methods that call it don't imply any non-UI thread safety. So I'm inclined to mark this bug as INVALID and leave the code as is (there are tons of places in the JFace code that simply assume that Display.getCurrent() is valid). If we wanted to start behaving better/more helpful, we could throw some kind of "wrong thread" exception here if we discover that the display is null. But I can tell you that there are tons of places where JFace does not check for null. Boris, do you know of more general bugs that discuss these kinds of failures? If we decide that this method *should* be supported from non-UI threads, then the fix is technically incorrect when the user is running with multiple X displays from the UI thread. This is a rare case, but I think we would probably first check if there is a current display, ie display = Display.getCurrent() == null ? Display.getDefault() : Display.getCurrent();
(In reply to comment #12) > The stack traces provided (comment 0, comment 2, comment 8) all involve a call > to a message dialog from a non-UI thread (either a modal context that is forked > or a job). In these situations, the caller should be invoking the message > dialog using an asyncExec (or syncExec if appropriate). I guess this wasn't > obvious to me when I first looked at this bug. I agree with this assessment. > If we simply cover up the NPE by using the default display, I would expect an > SWT "invalid thread access" failure shortly after. (Maybe that is more > informative!) The correct answer is that the client use an async (as Mik ended > up doing). I don't think we should cover up the NPE, but I agree that it is not as informative as you would like. > In looking at the code history, there was originally a bug in this area > involving an NPE when a null shell was passed in (bug 61743). That bug was > fixed and in doing so, the display.syncExec block was introduced. Honestly, > that block doesn't make any sense to me. Neither does it make sense to me. It was introduced back in 3.0 and never changed substantially since then. > I agree with Christopher that the syncExec used in getSWTImage seems to "imply" > that the icon retrieval methods should be non-UI thread safe. You should *not* rely on implementation details that you may glean from reading the code. If the JavaDoc is not clear on anything, you need to file a bug asking for a clarification. By relying on implementation details, you make yourself vulnerable to changes we may decide to introduce in the implementation, and/or you introduce "de facto" contracts of the kind "this has worked since 2.0 and now you are breaking my code" which over time make it impossible to change anything (perhaps including bug fixes!). > So I'm inclined to mark this bug as INVALID and leave the code as is (there are > tons of places in the JFace code that simply assume that Display.getCurrent() > is valid). Yes - with 20/20 hindsight we would probably have added asserts to all of our API methods early on, similar to the checks in SWT. Now it is too much work, and maybe also impossible because of the above mentioned "de facto" contracts to introduce something like this in 3.x. > If we wanted to start behaving better/more helpful, we could throw some kind of > "wrong thread" exception here if we discover that the display is null. But I > can tell you that there are tons of places where JFace does not check for null. Yes, there is always the question of consistency. However in the case at hand, IconAndMessageDialog.getSWTImage, we know that accessing it from a wrong thread will, under all circumstances, cause a NPE. Because of this, I propose that we add an assert like: Assert.isTrue(display != null, "Invalid access from non-UI thread."); > Boris, do you know of more general bugs that discuss these kinds of failures? No, sorry but I haven't seen a more general bug like this. > If we decide that this method *should* be supported from non-UI threads, then > the fix is technically incorrect when the user is running with multiple X > displays from the UI thread. This is a rare case, but I think we would > probably first check if there is a current display, ie > > display = Display.getCurrent() == null ? Display.getDefault() : > Display.getCurrent(); All instances where this "bug" was reported involved opening a dialog. I think it is safe to say that we don't need to support calling this method from a non-UI thread.
Created attachment 141602 [details] Another fix proposition I believe that this bug needs to be resolved because as a result you get NPE. This NPE indicates bug in jface, and if inspect the code you can find obvious inconsistency there (my survey says that you need 2 minutes to discover what's wrong). Adding assert is a good solution and I wish I have thought about this at first glance.
Created attachment 141603 [details] Another fix proposition I believe that this bug needs to be resolved because as a result you get NPE. This NPE indicates bug in jface, and if inspect the code you can find obvious inconsistency there (my survey says that you need 2 minutes to discover what's wrong). Adding assert is a good solution and I wish I have thought about this at first glance.
Is the latest patch releasable?
(In reply to comment #16) > Is the latest patch releasable? Yes, looks good.
Patch released. According to plan (http://www.eclipse.org/projects/project-plan.php?projectid=eclipse) we are in M2, but since I could not find M1 build or single bug fixed in M2 I have assumed that the plan is out of date. Please correct if necessary.
This fix causes bug 284856. Although it's not explicitly spec'd that the constructor of MessageDialog can be called from a non-UI thread, the Javadoc of the superclass org.eclipse.jface.dialogs.Dialog at least hints at this: /** * Creates a dialog instance. Note that the window will have no visual * representation (no widgets) until it is told to open. [..] I don't think it's worth breaking existing clients like this after years. I would go with the fix proposed at the end of comment 12, since it *was* possible to instantiate a MessageDialog from a non-UI thread before -- only calling open() from a non-UI thread failed.
(In reply to comment #18) > ... but since I could not find M1 build or single bug fixed in M2 I have assumed > that the plan is out of date. Please correct if necessary. M1 is 07 Aug.
(In reply to comment #19) > This fix causes bug 284856. Although it's not explicitly spec'd that the > constructor of MessageDialog can be called from a non-UI thread, the Javadoc of > the superclass org.eclipse.jface.dialogs.Dialog at least hints at this: > /** > * Creates a dialog instance. Note that the window will have no visual > * representation (no widgets) until it is told to open. [..] > > I don't think it's worth breaking existing clients like this after years. > > I would go with the fix proposed at the end of comment 12, since it *was* > possible to instantiate a MessageDialog from a non-UI thread before -- only > calling open() from a non-UI thread failed. We can address issues in Eclipse SDK, however there may be other consumers of MessageDialog relying on the old behavior. The current fix requires a note in the migration guide at least. And of course the one who releases such changes fixes broken code ;-)
Actually the patch does not strengthen method preconditions, it only fails fast. Creating MessageDialog in non-UI thread is risky because disposing appropriate shell in the appriopriate moment will cause NPE. For bug 284856 it is not very probable, but others are not so lucky. And because MessageDialog is affected - user misses important informations (stacktrace is logged but nothing is displayed). It is not a problem for me to provide subsequent bug-fixes.
I'm just saying that something used to work before the fix. If there are other clients which may be broken, this should be mentioned in the migration guide.
It took me a few minutes to realize the team code was working because the shell was good, so the proper display is used for the syncExec. That's probably the case that caused the syncExec code to be added. (just guessing). I think the original bug isn't important enough that we force clients to fix their code. It will waste a fair bit of time to track down the cause of these kinds of bugs, plus explaining it in the migration guide, etc. Where the only reason for doing so is to be more "pure/strict". I think the best fix is to add an assert that will catch only the cases where the NPE would have happened anyway. So just before trying to call the syncExec, do the assertion there. That way the cases that happened to work will still work, and those that NPE will provide a more descriptive reason.
Created attachment 142880 [details] NPE trapped This fix throws assertion before NPE happens.
(In reply to comment #24) > I think the best fix is to add an assert that will catch only the cases where > the NPE would have happened anyway. So just before trying to call the > syncExec, do the assertion there. That way the cases that happened to work > will still work, and those that NPE will provide a more descriptive reason. +1 - I didn't realize that we would be breaking clients who called the constructor on a non-UI thread but then called open from the UI thread.
+1 on new patch
"NPE trapped" released.
Verified by code inspection.
