Bug 336679

Comment 1 Dani Megert 2011-02-09 03:21:47 EST

(In reply to comment #0)
> Created attachment 188568 [details]
> But judging from the stacktrace, the error doesn't seem to
> come from Mylyn.
Hard to say without seeing the bottom of the stack. Mylyn should investigate that first.

Comment 2 Prakash Rangaraj 2011-02-09 04:12:06 EST

cc-ing Paul

Comment 3 Paul Webster 2011-02-09 08:10:44 EST

A CommandAction is a legacy representation of a Command (it simply forwards executing to the command).

The standard actionBarAdvisor.register(IAction) filters out CommandActions to avoid loops, as does actionBars.setGlobalActionHandler(*).

If a part of the system gets a hold of one (they're created by the ActionFactory, IDEActionFactory) and manually adds it to the system:

handlerService.activateHandler(new ActionHandler(commandAction)); you'll get this problem.

PW

Comment 4 Steffen Pingel 2011-02-09 18:47:38 EST

I wonder if this is related to bug 321701: [api] Support common keyboard commands for textual navigation . David, we should investigate for 3.5.

Comment 5 David Green 2011-02-10 13:49:24 EST

Thanks for the bug.  Nicolas, is this problem readily reproducible?  If so, can you provide the steps required to reproduce?  How did you initiate the save?

Steffen, from what I can see the code added in bug 321701 just registers command handlers using the normal <handler> extension point.  Was there specific code that you're questioning?  Perhaps I've missed something.

I did a quick search on activateHandler and found that @MarkupEditor@ has the following:

bc. 
	@Override
	public void setAction(String actionID, IAction action) {
		if (action.getActionDefinitionId() != null) {
			IHandlerService handlerService = (IHandlerService) getSite().getService(IHandlerService.class);
			handlerService.activateHandler(action.getActionDefinitionId(), new ActionHandler(action));
		}
		super.setAction(actionID, action);
	}
	
This code appears to have the same pattern as the referenced by Paul in comment #3, though the code has been that way since about 2008; strange that we'd see this problem now after all of that time.
Should it be checking to see if the action argument is a CommandAction?

Comment 6 David Green 2011-02-10 13:49:45 EST

Created attachment 188713 [details]
mylyn/context/zip

Comment 7 Paul Webster 2011-02-10 14:09:15 EST

(In reply to comment #5)

> This code appears to have the same pattern as the referenced by Paul in comment
> #3, though the code has been that way since about 2008; strange that we'd see
> this problem now after all of that time.
> Should it be checking to see if the action argument is a CommandAction?

The Text editor framework follows a similar pattern, but its setAction(*) delegates (through a couple of classes) to a legacy KeyBindingService which has the same protection as the actionBarAdvisor:
if (action instanceof CommandAction) {
	// we unfortunately had to allow these out into the wild, but they
	// still must not feed back into the system
	return;
}


My guess is this wouldn't show up until some other part of the system tried to feed the command action into your editor.

If you can reproduce you can find out the ID and definition ID of the action in question ... that will give a hint of where it's coming from.

PW

Comment 8 Nicolas Bros 2011-02-11 03:36:44 EST

Created attachment 188750 [details]
stackdump when hitting "action instanceof CommandAction" case in KeyBindingService

I could reproduce:
- open the Mylyn MarkupEditor on a ".wikimedia" file
- modify the file
- you can't save directly, because the save action is disabled (see Bug 336681 - cannot save .mediawiki file)
- move the focus to another view (I use the error log view), and then back to the Mylyn MarkupEditor, which re-enables the save action
- save (File > Save or Ctrl+S)

I put a breakpoint in KeyBindingService, in the code highlighted by Paul.
I get actionDefinitionID = "org.eclipse.ui.file.save"
I am attaching the stack dump.

Comment 9 David Green 2011-03-03 00:27:52 EST

thanks for the help, fixed.

Comment 10 David Green 2011-03-03 00:27:57 EST

Created attachment 190233 [details]
mylyn/context/zip