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

Bug 340886

Summary: NPE in handlers
Product: z_Archived Reporter: Nicolas Bros <nicolas.bros>
Component: EMF-FacetAssignee: Nicolas Bros <nicolas.bros>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P4 CC: Ed.Merks, ed, fabien.giquel, gdupe, modisco.web-inbox
Version: unspecifiedFlags: nicolas.bros: indigo+
Ed.Merks: pmc_approved+
fabien.giquel: review+
gdupe: review+
Target Milestone: ---   
Hardware: PC   
OS: Mac OS X - Carbon (unsup.)   
Whiteboard:
Attachments:
Description Flags
Patch for Bug 340886 RemoveLine Handler
none
Patch Bug 340886 NPE in handlers nicolas.bros: iplog+

Description Nicolas Bros CLA 2011-03-24 13:03:28 EDT
I have a NPE in the following classes when exiting Eclipse (the active page is null when exiting) :
org.eclipse.emf.facet.widgets.nattable.internal.handlers.RemoveLineHandler
org.eclipse.emf.facet.widgets.nattable.internal.handlers.CreateNewElementHandler
Comment 1 Ed Willink CLA 2011-05-30 07:10:32 EDT
Me too, and I didn't know that I had EMF Facet installed; maybe Papyrus does it.

Anyway NPE still present in RC2. Shouldn't this be "major" and could shutdown activity be bypassed?
Comment 2 Nicolas Guyomar CLA 2011-05-30 07:32:14 EDT
Created attachment 196881 [details]
Patch for Bug 340886 RemoveLine Handler

Hi,

Indeed, those NPE should have been taken care of.
If I add a breakpoint on NPE and close my Eclipse, I only detect one handler with this problem.

Here is a patch for org.eclipse.emf.facet.widgets.nattable.internal.handlers.RemoveLineHandler.java to catch this NPE and return false in isEnabled() method

(a) I, Nicolas Guyomar, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Regards,
Nicolas Guyomar
Comment 3 Ed Willink CLA 2011-05-30 07:40:31 EDT
Catching NPEs from known causes is rather inefficient.

The problem is that getActivePage() returns null, so why not test the null return, rather than inflict an unnecessary exception dispatch.
Comment 4 Nicolas Guyomar CLA 2011-05-30 07:55:24 EDT
Created attachment 196883 [details]
Patch Bug 340886 NPE in handlers

Hi Edward,

You are absolutely right, I only wanted to minimize code modification for RC3.

Here is a patch to remove unnecessary try/catch  blocks from our handlers, and replace them with null test.

CopyHandler.java
CreateNewElementHandler.java
DeleteHandler.java
ExportCommandHandler.java
RemoveLineHandler.java

(a) I, Nicolas Guyomar, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Regards,
Nicolas Guyomar
Comment 5 Gregoire Dupe CLA 2011-05-30 11:46:18 EDT
Hello Ed (Merks),

I don’t know if this bug is major. No messages are sent to the log and no error pops up. The only way to check this bug is to add a breakpoint on java.lang.NullPointerException.

I’ve checked the patch: it looks good and it does not break the unit tests.

Is this bug major? Must we commit the patch before RC3?

Regards,
Gregoire
Comment 6 Ed Willink CLA 2011-05-30 11:54:10 EDT
(In reply to comment #5)
> Is this bug major? 

No. I was forgetting that users don't see it.

> Must we commit the patch before RC3?

An easy fix, much better at RC3 than RC4.

It's a pain for developers. I like to watch all NPE and CCE since they're the most insidious, and with spurious NPEs coming from handlers many interactive debug activities are hindered.
Comment 7 Ed Merks CLA 2011-05-30 12:03:13 EDT
If the risk of causing problems is low, then it seems good to fix it.
Comment 8 Gregoire Dupe CLA 2011-05-30 12:06:40 EDT
Fabien,

Please, can you check this patch ?

Regards,
Grégoire
Comment 9 Nicolas Bros CLA 2011-05-31 03:06:16 EDT
Comment on attachment 196883 [details]
Patch Bug 340886 NPE in handlers

This is a contribution from an employee of Mia-Software, targeting a future
Indigo release. The company has signed a Member Committer Agreement. The
contribution does not need a CQ (see bug 322327).
Comment 10 Nicolas Bros CLA 2011-05-31 03:07:09 EDT
Committed in revision 706.
Comment 11 Gregoire Dupe CLA 2011-06-06 10:15:33 EDT
This bug can be closed.
Comment 12 Nicolas Bros CLA 2011-08-19 08:51:09 EDT
There are still NPEs in the RemoveLineHandler, that are invisible in Eclipse 3.x, but cause issues in 4.x.
See Bug 354967 comment 7:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=354967#c7
Comment 13 Nicolas Bros CLA 2011-08-19 09:04:00 EDT
I also get a NPE here:

Thread [main] (Suspended (exception NullPointerException))	
	CreateNewElementHandler.isEnabled() line: 52	
	HandlerProxy.isEnabled() line: 320	
	Command.isEnabled() line: 833	
	Command.setHandler(IHandler) line: 965	
	HandlerAuthority.updateCommand(String, IHandlerActivation) line: 459	
	HandlerAuthority.processChangedCommands() line: 638	
	HandlerAuthority.access$1(HandlerAuthority) line: 610	
	HandlerAuthority$1.propertyChange(PropertyChangeEvent) line: 175	
	EvaluationAuthority$1.run() line: 252	
	SafeRunner.run(ISafeRunnable) line: 42	
	EvaluationAuthority.fireServiceChange(String, Object, Object) line: 246	
	EvaluationAuthority.endSourceChange(String[]) line: 197	
	EvaluationAuthority.sourceChanged(String[]) line: 135	
	EvaluationAuthority(ExpressionAuthority).sourceChanged(int, String[]) line: 311	
	EvaluationAuthority(ExpressionAuthority).sourceChanged(int, Map) line: 290	
	WorkbenchSourceProvider(AbstractSourceProvider).fireSourceChanged(int, Map) line: 99	
	WorkbenchSourceProvider.checkActivePart(boolean) line: 401	
	WorkbenchSourceProvider.checkActivePart() line: 300	
	WorkbenchSourceProvider.handleCheck(Shell) line: 286	
	WorkbenchSourceProvider.checkOtherSources(Shell) line: 855	
	WorkbenchSourceProvider$6.handleEvent(Event) line: 839	
	EventTable.sendEvent(Event) line: 84	
	Display.filterEvent(Event) line: 1262	
	Shell(Widget).sendEvent(Event) line: 1052	
	Shell(Widget).sendEvent(int, Event, boolean) line: 1077	
	Shell(Widget).sendEvent(int) line: 1058	
	Shell(Decorations).WM_ACTIVATE(long, long) line: 1647	
	Shell.WM_ACTIVATE(long, long) line: 2137	
	Shell(Control).windowProc(long, int, long, long) line: 4525	
	Shell(Canvas).windowProc(long, int, long, long) line: 341	
	Shell(Decorations).windowProc(long, int, long, long) line: 1610	
	Shell.windowProc(long, int, long, long) line: 2061	
	Display.windowProc(long, long, long, long) line: 4972	
	OS.DestroyWindow(long) line: not available [native method]	
	Shell(Control).destroyWidget() line: 780	
	Shell.destroyWidget() line: 698	
	Shell(Widget).release(boolean) line: 818	
	Shell(Widget).dispose() line: 446	
	Shell(Decorations).dispose() line: 447	
	Shell.dispose() line: 715	
	SaveablesList$3(Window).close() line: 335	
	SaveablesList$3(Dialog).close() line: 979	
	SaveablesList$3(MessageDialog).buttonPressed(int) line: 207	
	Dialog$2.widgetSelected(SelectionEvent) line: 624	
	TypedListener.handleEvent(Event) line: 240	
	EventTable.sendEvent(Event) line: 84	
	Button(Widget).sendEvent(Event) line: 1053	
	Display.runDeferredEvents() line: 4165	
	Display.readAndDispatch() line: 3754	
	SaveablesList$3(Window).runEventLoop(Shell) line: 825	
	SaveablesList$3(Window).open() line: 801	
	SaveablesList$3(MessageDialog).open() line: 334	
	SaveablesList.promptForSaving(List, IShellProvider, IRunnableContext, boolean, boolean) line: 529	
	SaveablesList.promptForSavingIfNecessary(IShellProvider, IWorkbenchWindow, Set, Map, boolean) line: 433	
	SaveablesList.preCloseParts(List, boolean, IShellProvider, IWorkbenchWindow) line: 388	
	SaveablesList.preCloseParts(List, boolean, IWorkbenchWindow) line: 347	
	WorkbenchPage.closeEditors(IEditorReference[], boolean) line: 1426	
	WorkbenchPage.closeEditor(IEditorReference, boolean) line: 1514	
	EditorPane.doHide() line: 61	
	EditorStack(PartStack).close(IPresentablePart) line: 537	
	EditorStack.close(IPresentablePart[]) line: 206	
	PartStack$1.close(IPresentablePart[]) line: 120	
	TabbedStackPresentation$1.handleEvent(TabFolderEvent) line: 83	
	DefaultTabFolder(AbstractTabFolder).fireEvent(TabFolderEvent) line: 269	
	DefaultTabFolder(AbstractTabFolder).fireEvent(int, AbstractTabItem) line: 278	
	DefaultTabFolder.access$1(DefaultTabFolder, int, AbstractTabItem) line: 1	
	DefaultTabFolder$1.closeButtonPressed(CTabItem) line: 71	
	PaneFolder.notifyCloseListeners(CTabItem) line: 631	
	PaneFolder$3.close(CTabFolderEvent) line: 206	
	CTabFolder.onMouse(Event) line: 1598	
	CTabFolder$1.handleEvent(Event) line: 261	
	EventTable.sendEvent(Event) line: 84	
	CTabFolder(Widget).sendEvent(Event) line: 1053	
	Display.runDeferredEvents() line: 4165	
	Display.readAndDispatch() line: 3754	
	Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2696	
	Workbench.runUI() line: 2660	
	Workbench.access$4(Workbench) line: 2494	
	Workbench$7.run() line: 674	
	Realm.runWithDefault(Realm, Runnable) line: 332	
	Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 667	
	PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149	
	IDEApplication.start(IApplicationContext) line: 123	
	EclipseAppHandle.run(Object) line: 196	
	EclipseAppLauncher.runApplication(Object) line: 110	
	EclipseAppLauncher.start(Object) line: 79	
	EclipseStarter.run(Object) line: 344	
	EclipseStarter.run(String[], Runnable) line: 179	
	NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]	
	NativeMethodAccessorImpl.invoke(Object, Object[]) line: 39	
	DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 25	
	Method.invoke(Object, Object...) line: 597	
	Main.invokeFramework(String[], URL[]) line: 622	
	Main.basicRun(String[]) line: 577	
	Main.run(String[]) line: 1410	
	Main.main(String[]) line: 1386
Comment 14 Gregoire Dupe CLA 2012-05-23 12:29:04 EDT
This bug has to be closed for the IP log.
Comment 15 Ed Willink CLA 2012-05-23 12:35:06 EDT
Sibnce this causes users considerable irritation, couldn't you fix it before closing it?? It seems very wrong to close an open bug.
Comment 16 Gregoire Dupe CLA 2012-05-25 09:53:41 EDT
(In reply to comment #15)
> Sibnce this causes users considerable irritation, couldn't you fix it before
> closing it?? It seems very wrong to close an open bug.

I hadn't any other choices: it is a constraint of the IP log generator, but I've open a new set of bugs : cf. https://bugs.eclipse.org/bugs/buglist.cgi?query_format=advanced;list_id=1737552;short_desc=NullPointerException;bug_status=UNCONFIRMED;bug_status=NEW;bug_status=ASSIGNED;bug_status=REOPENED;short_desc_type=allwordssubstr;product=EMFT.facet