Comment 1 Szymon Ptaszkiewicz 2014-01-30 10:45:44 EST

(In reply to Szymon Ptaszkiewicz from comment #0)
Point 4 is a NOP of course ;-)

Comment 2 Szymon Ptaszkiewicz 2014-01-31 04:46:55 EST

Interesting thing is that NPE reported by the error dialog is different than NPE logged in .log. Here is the stack of the NPE reported by the error dialog (which happens before NPE logged in .log):

Thread [main] (Suspended (exception java.lang.NullPointerException))	
	org.eclipse.debug.internal.ui.views.launch.TerminateAndRemoveHandler(org.eclipse.debug.internal.ui.commands.actions.DebugActionHandler).getDelegate() line: 48	
	org.eclipse.debug.internal.ui.views.launch.TerminateAndRemoveHandler(org.eclipse.debug.internal.ui.commands.actions.DebugActionHandler).addHandlerListener(org.eclipse.core.commands.IHandlerListener) line: 76	
	org.eclipse.ui.internal.handlers.HandlerProxy.loadHandler() line: 348	
	org.eclipse.ui.internal.handlers.HandlerProxy.setEnabled(java.lang.Object) line: 230	
	org.eclipse.ui.internal.handlers.E4HandlerProxy.canExecute(org.eclipse.e4.core.contexts.IEclipseContext, org.eclipse.core.expressions.IEvaluationContext, org.eclipse.e4.ui.model.application.MApplication) line: 71	
	sun.reflect.GeneratedMethodAccessor9.invoke(java.lang.Object, java.lang.Object[]) line: not available	
	sun.reflect.DelegatingMethodAccessorImpl.invoke(java.lang.Object, java.lang.Object[]) line: 37	
	java.lang.reflect.Method.invoke(java.lang.Object, java.lang.Object...) line: 611	
	org.eclipse.e4.core.internal.di.MethodRequestor.execute() line: 56	
	org.eclipse.e4.core.internal.di.InjectorImpl.invokeUsingClass(java.lang.Object, java.lang.Class<?>, java.lang.Class<? extends java.lang.annotation.Annotation>, java.lang.Object, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, boolean) line: 243	
	org.eclipse.e4.core.internal.di.InjectorImpl.invoke(java.lang.Object, java.lang.Class<? extends java.lang.annotation.Annotation>, java.lang.Object, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier) line: 224	
	org.eclipse.e4.core.contexts.ContextInjectionFactory.invoke(java.lang.Object, java.lang.Class<? extends java.lang.annotation.Annotation>, org.eclipse.e4.core.contexts.IEclipseContext, org.eclipse.e4.core.contexts.IEclipseContext, java.lang.Object) line: 132	
	org.eclipse.ui.internal.WorkbenchHandlerServiceHandler(org.eclipse.e4.core.commands.internal.HandlerServiceHandler).isEnabled() line: 55	
	org.eclipse.core.commands.Command.isEnabled() line: 862	
	org.eclipse.ui.internal.handlers.LegacyHandlerService.registerLegacyHandler(org.eclipse.e4.core.contexts.IEclipseContext, java.lang.String, java.lang.String, org.eclipse.core.commands.IHandler, org.eclipse.core.expressions.Expression) line: 154	
	org.eclipse.ui.internal.handlers.LegacyHandlerService.activateHandler(java.lang.String, org.eclipse.core.commands.IHandler, org.eclipse.core.expressions.Expression, boolean) line: 309	
	org.eclipse.ui.internal.handlers.LegacyHandlerService.activateHandler(java.lang.String, org.eclipse.core.commands.IHandler, org.eclipse.core.expressions.Expression) line: 290	
	org.eclipse.ui.internal.menus.LegacyActionPersistence.convertActionToHandler(org.eclipse.core.runtime.IConfigurationElement, java.lang.String, org.eclipse.core.commands.ParameterizedCommand, org.eclipse.core.expressions.Expression, java.lang.String, java.util.List) line: 376	
	org.eclipse.ui.internal.menus.LegacyActionPersistence.readActions(java.lang.String, org.eclipse.core.runtime.IConfigurationElement[], java.util.List, org.eclipse.core.expressions.Expression, java.lang.String) line: 523	
	org.eclipse.ui.internal.menus.LegacyActionPersistence.readActionsAndMenus(org.eclipse.core.runtime.IConfigurationElement, java.lang.String, java.util.List, org.eclipse.core.expressions.Expression, java.lang.String) line: 563	
	org.eclipse.ui.internal.menus.LegacyActionPersistence.readViewContributions(org.eclipse.core.runtime.IConfigurationElement[], int) line: 715	
	org.eclipse.ui.internal.menus.LegacyActionPersistence.read() line: 469	
	org.eclipse.ui.internal.WorkbenchWindow.initializeDefaultServices() line: 2734	
	org.eclipse.ui.internal.WorkbenchWindow.setup() line: 601	
	sun.reflect.NativeMethodAccessorImpl.invoke0(java.lang.reflect.Method, java.lang.Object, java.lang.Object[]) line: not available [native method]	
	sun.reflect.NativeMethodAccessorImpl.invoke(java.lang.Object, java.lang.Object[]) line: 60	
	sun.reflect.DelegatingMethodAccessorImpl.invoke(java.lang.Object, java.lang.Object[]) line: 37	
	java.lang.reflect.Method.invoke(java.lang.Object, java.lang.Object...) line: 611	
	org.eclipse.e4.core.internal.di.MethodRequestor.execute() line: 56	
	org.eclipse.e4.core.internal.di.InjectorImpl.processAnnotated(java.lang.Class<? extends java.lang.annotation.Annotation>, java.lang.Object, java.lang.Class<?>, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, java.util.ArrayList<java.lang.Class<?>>) line: 877	
	org.eclipse.e4.core.internal.di.InjectorImpl.inject(java.lang.Object, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier) line: 119	
	org.eclipse.e4.core.internal.di.InjectorImpl.inject(java.lang.Object, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier) line: 84	
	org.eclipse.e4.core.contexts.ContextInjectionFactory.inject(java.lang.Object, org.eclipse.e4.core.contexts.IEclipseContext) line: 73	
	org.eclipse.ui.internal.Workbench.createWorkbenchWindow(org.eclipse.core.runtime.IAdaptable, org.eclipse.ui.IPerspectiveDescriptor, org.eclipse.e4.ui.model.application.ui.basic.MWindow, boolean) line: 1424	
	org.eclipse.ui.internal.Workbench.openWorkbenchWindow(org.eclipse.core.runtime.IAdaptable, org.eclipse.ui.IPerspectiveDescriptor, org.eclipse.e4.ui.model.application.ui.basic.MWindow, boolean) line: 2524	
	org.eclipse.ui.internal.Workbench.openWorkbenchWindow(java.lang.String, org.eclipse.core.runtime.IAdaptable) line: 2516	
	org.eclipse.ui.internal.handlers.OpenInNewWindowHandler.execute(org.eclipse.core.commands.ExecutionEvent) line: 58	
	org.eclipse.ui.internal.handlers.HandlerProxy.execute(org.eclipse.core.commands.ExecutionEvent) line: 290	
	org.eclipse.ui.internal.handlers.E4HandlerProxy.execute(org.eclipse.e4.core.contexts.IEclipseContext, java.util.Map, org.eclipse.swt.widgets.Event, org.eclipse.core.expressions.IEvaluationContext) line: 90	
	sun.reflect.NativeMethodAccessorImpl.invoke0(java.lang.reflect.Method, java.lang.Object, java.lang.Object[]) line: not available [native method]	
	sun.reflect.NativeMethodAccessorImpl.invoke(java.lang.Object, java.lang.Object[]) line: 60	
	sun.reflect.DelegatingMethodAccessorImpl.invoke(java.lang.Object, java.lang.Object[]) line: 37	
	java.lang.reflect.Method.invoke(java.lang.Object, java.lang.Object...) line: 611	
	org.eclipse.e4.core.internal.di.MethodRequestor.execute() line: 56	
	org.eclipse.e4.core.internal.di.InjectorImpl.invokeUsingClass(java.lang.Object, java.lang.Class<?>, java.lang.Class<? extends java.lang.annotation.Annotation>, java.lang.Object, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, boolean) line: 243	
	org.eclipse.e4.core.internal.di.InjectorImpl.invoke(java.lang.Object, java.lang.Class<? extends java.lang.annotation.Annotation>, java.lang.Object, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier, org.eclipse.e4.core.di.suppliers.PrimaryObjectSupplier) line: 224	
	org.eclipse.e4.core.contexts.ContextInjectionFactory.invoke(java.lang.Object, java.lang.Class<? extends java.lang.annotation.Annotation>, org.eclipse.e4.core.contexts.IEclipseContext, org.eclipse.e4.core.contexts.IEclipseContext, java.lang.Object) line: 132	
	org.eclipse.ui.internal.WorkbenchHandlerServiceHandler(org.eclipse.e4.core.commands.internal.HandlerServiceHandler).execute(org.eclipse.core.commands.ExecutionEvent) line: 153	
	org.eclipse.core.commands.Command.executeWithChecks(org.eclipse.core.commands.ExecutionEvent) line: 499	
	org.eclipse.core.commands.ParameterizedCommand.executeWithChecks(java.lang.Object, java.lang.Object) line: 508	
	org.eclipse.e4.core.commands.internal.HandlerServiceImpl.executeHandler(org.eclipse.core.commands.ParameterizedCommand, org.eclipse.e4.core.contexts.IEclipseContext) line: 222	
	org.eclipse.ui.internal.handlers.LegacyHandlerService.executeCommand(org.eclipse.core.commands.ParameterizedCommand, org.eclipse.swt.widgets.Event) line: 420	
	org.eclipse.ui.actions.ActionFactory$WorkbenchCommandAction(org.eclipse.ui.internal.actions.CommandAction).runWithEvent(org.eclipse.swt.widgets.Event) line: 157	
	org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(org.eclipse.swt.widgets.Event, boolean) line: 588	
	org.eclipse.jface.action.ActionContributionItem.access$2(org.eclipse.jface.action.ActionContributionItem, org.eclipse.swt.widgets.Event, boolean) line: 505	
	org.eclipse.jface.action.ActionContributionItem$5.handleEvent(org.eclipse.swt.widgets.Event) line: 415	
	org.eclipse.swt.widgets.EventTable.sendEvent(org.eclipse.swt.widgets.Event) line: 84	
	org.eclipse.swt.widgets.Display.sendEvent(org.eclipse.swt.widgets.EventTable, org.eclipse.swt.widgets.Event) line: 4353	
	org.eclipse.swt.widgets.MenuItem(org.eclipse.swt.widgets.Widget).sendEvent(org.eclipse.swt.widgets.Event) line: 1061	
	org.eclipse.swt.widgets.Display.runDeferredEvents() line: 4172	
	org.eclipse.swt.widgets.Display.readAndDispatch() line: 3761	
	org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$9.run() line: 1122	
	org.eclipse.core.databinding.observable.Realm.runWithDefault(org.eclipse.core.databinding.observable.Realm, java.lang.Runnable) line: 332	
	org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(org.eclipse.e4.ui.model.application.MApplicationElement, org.eclipse.e4.core.contexts.IEclipseContext) line: 1006	
	org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(org.eclipse.e4.ui.model.application.MApplicationElement) line: 147	
	org.eclipse.ui.internal.Workbench$5.run() line: 620	
	org.eclipse.core.databinding.observable.Realm.runWithDefault(org.eclipse.core.databinding.observable.Realm, java.lang.Runnable) line: 332	
	org.eclipse.ui.internal.Workbench.createAndRunWorkbench(org.eclipse.swt.widgets.Display, org.eclipse.ui.application.WorkbenchAdvisor) line: 571	
	org.eclipse.ui.PlatformUI.createAndRunWorkbench(org.eclipse.swt.widgets.Display, org.eclipse.ui.application.WorkbenchAdvisor) line: 150	
	org.eclipse.ui.internal.ide.application.IDEApplication.start(org.eclipse.equinox.app.IApplicationContext) line: 125	
	org.eclipse.equinox.internal.app.EclipseAppHandle.run(java.lang.Object) line: 196	
	org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(java.lang.Object) line: 109	
	org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(java.lang.Object) line: 80	
	org.eclipse.core.runtime.adaptor.EclipseStarter.run(java.lang.Object) line: 372	
	org.eclipse.core.runtime.adaptor.EclipseStarter.run(java.lang.String[], java.lang.Runnable) line: 226	
	sun.reflect.NativeMethodAccessorImpl.invoke0(java.lang.reflect.Method, java.lang.Object, java.lang.Object[]) line: not available [native method]	
	sun.reflect.NativeMethodAccessorImpl.invoke(java.lang.Object, java.lang.Object[]) line: 60	
	sun.reflect.DelegatingMethodAccessorImpl.invoke(java.lang.Object, java.lang.Object[]) line: 37	
	java.lang.reflect.Method.invoke(java.lang.Object, java.lang.Object...) line: 611	
	org.eclipse.equinox.launcher.Main.invokeFramework(java.lang.String[], java.net.URL[]) line: 648	
	org.eclipse.equinox.launcher.Main.basicRun(java.lang.String[]) line: 603	
	org.eclipse.equinox.launcher.Main.run(java.lang.String[]) line: 1462	
	org.eclipse.equinox.launcher.Main.main(java.lang.String[]) line: 1438	

This NPE comes from org.eclipse.debug.* but there were no changes in that class recently and it works fine in 3.8.2 so there must be something e4-specific that causes the problem.

Comment 3 Szymon Ptaszkiewicz 2014-01-31 04:58:26 EST

I will take this one.

Comment 4 Szymon Ptaszkiewicz 2014-01-31 11:44:41 EST

The reason why we get NPE from org.eclipse.debug.* (comment 2) is that the handler of the org.eclipse.ui.edit.delete command is taken from wrong execution context. HandlerServiceHandler.isEnabled() takes execution context from the context stack and since during execution of the New Window command we are in the context of recently active view - Debug view, it is Debug's view context that is taken as execution context. This means that org.eclipse.ui.edit.delete is resolved as if it happened inside Debug view thus org.eclipse.debug.internal.ui.views.launch.TerminateAndRemoveHandler is pinged.

The solution is to push the new window's context on the HandlerServiceHandler's context stack for the time when new windows is created, so that any command that is touched during the creation of the new window is run in the context of the new window. The simplest fix looks like this:

	try {
		HandlerServiceImpl.push(windowContext, null);
		initializeDefaultServices();
	} finally {
		HandlerServiceImpl.pop();
	}

but we could also consider wrapping in try-finally the whole body of WorkbenchWindow.setup() in case commands can be run outside initializeDefaultServices but still during new window creation.

Subsequent NPE logged in .log (comment 0) is a side effect of the above NPE.

Comment 5 Szymon Ptaszkiewicz 2014-01-31 12:15:39 EST

The problem with solution proposed in comment 4 is that it uses static methods of internal class from org.eclipse.e4.core.commands. It would be better if we could pass current execution context as an argument (see HandlerServiceHandler.execute(ExecutionEvent)) but unfortunately HandlerServiceHandler.isEnabled() doesn't take any parameters.

We could also extend EHandlerService with push and pop methods and then use it without using internals, but I'm not sure if we want push and pop methods published as API.

Paul, what would be the recommended approach?

Comment 6 Szymon Ptaszkiewicz 2014-02-03 07:53:28 EST

There is already a precedence of using static internal methods from e4 commands plugin, so I chose the same approach. Pushed the fix for review:

https://git.eclipse.org/r/#/c/21459/

Comment 7 Paul Webster 2014-02-04 15:04:03 EST

(In reply to Szymon Ptaszkiewicz from comment #4)
> The reason why we get NPE from org.eclipse.debug.* (comment 2) is that the
> handler of the org.eclipse.ui.edit.delete command is taken from wrong
> execution context. HandlerServiceHandler.isEnabled() takes execution context
> from the context stack and since during execution of the New Window command
> we are in the context of recently active view - Debug view, it is Debug's
> view context that is taken as execution context. This means that
> org.eclipse.ui.edit.delete is resolved as if it happened inside Debug view
> thus org.eclipse.debug.internal.ui.views.launch.TerminateAndRemoveHandler is
> pinged.

So because expressions haven't been evaluated, the current handler is window1 handler? 

The fix seems reasonable, but I'm worried about 1) the context used for handler evaluation won't match the runtime contexts anymore since the new window hasn't been fully realized yet and 2) the new window context is it connected up to anything (I think yes, but it's not the active child of its parent).

I've asked Paul E to review this as well.

PW

Comment 8 Szymon Ptaszkiewicz 2014-02-05 09:35:24 EST

(In reply to Paul Webster from comment #7)
> So because expressions haven't been evaluated, the current handler is
> window1 handler? 

I'm not sure I understand correctly the question so I will say this: the current context (the one against which handler is evaluated) is the context that was put on the stack before executing the New Window command, which is the original window's context.

> The fix seems reasonable, but I'm worried about 1) the context used for
> handler evaluation won't match the runtime contexts anymore since the new
> window hasn't been fully realized yet

I think the context object is the same but the state of the object may be different between the time of handler evaluation and later when window is realized. Isn't this kind of chicken-and-egg problem? Shouldn't we start evaluating handlers when window is already fully realized?

> and 2) the new window context is it
> connected up to anything (I think yes, but it's not the active child of its
> parent).

I've been thinking about it and I think the new window context should be activated at the time active workbench window is changed. Not sure how it works now.

Comment 9 Paul Elder 2014-02-05 15:06:39 EST

The first time org.eclipse.ui.internal.WorkbenchWindow.setup() is called (as the workbench is created), the HandlerServiceImpl context stack contains one entry, the Workbench Context.

The, the second call the WorkbenchWindow.setup(), the HandlerServiceImpl context stack contains Workbench Context, and a second entry (the top) corresponding to the context a 'PageSite' context contained by the the Debug View's context. This entry was pushed by HandlerServiceImpl.executeHandler as it executed the Window -> New Window action.

It seems to me that we shoud make WorkbenchWindow.setup() behave consistently, so I think the patch should be changed as follows:

* the try for HandlerServiceImpl.push() should contain the whole (or most of?) the setup() method
* the context that is pushed should be the workbench context, not the new window's context.

Comment 10 Szymon Ptaszkiewicz 2014-02-06 07:52:30 EST

Pushed a new patch set according to changes suggested in comment 9.

Comment 11 Paul Elder 2014-02-06 09:49:59 EST

(In reply to Szymon Ptaszkiewicz from comment #10)
> Pushed a new patch set according to changes suggested in comment 9.

Looks good. Release on master as:

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f51d436e1980f94edd708263ac31285dfbde3a0b

Comment 12 Szymon Ptaszkiewicz 2014-02-12 05:07:26 EST

The fix was pushed last week (see comment 9).

Comment 13 Szymon Ptaszkiewicz 2014-02-12 05:08:23 EST

(In reply to Szymon Ptaszkiewicz from comment #12)
> The fix was pushed last week (see comment 9).

Typo: should be "see comment 11".

Verified in I20140211-1100.