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

Bug 331337

Summary:

[Wizards] Bad implementation of IWizardPage's dispose() method can prevent the dialog from closing

Product:

[Eclipse Project] Platform

Reporter:

Remy Suen <remy.suen>

Component:

Assignee:

Prakash Rangaraj <prakash>

Status:

VERIFIED FIXED

QA Contact:

Prakash Rangaraj <prakash>

Severity:

major

Priority:

CC:

hsoliwal, prakash

Version:

3.7

Target Milestone:

3.7 M5

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Patch v01	none

Description Remy Suen

2010-11-29 08:30:12 EST

The IWizardPage variant of bug 331335. Incorrect implementations of IWizardPage's dispose() will cause the dialog to stay up forever.

-----------

Display display = new Display();
Wizard wizard = new Wizard() {
  public void addPages() {
    WizardPage page = new WizardPage("") { //$NON-NLS-1$
      public void createControl(Composite parent) {
        setControl(new Composite(parent, SWT.NONE));
      }
		
      public void dispose() {
        throw new RuntimeException();
      }
    };
    addPage(page);
  }

  public boolean performFinish() {
    return true;
  }
};
WizardDialog dialog = new WizardDialog(null, wizard);
dialog.open();
display.dispose();

-----------

java.lang.RuntimeException
	at Main$6.dispose(Main.java:144)
	at org.eclipse.jface.wizard.Wizard.dispose(Wizard.java:188)
	at org.eclipse.jface.wizard.WizardDialog.hardClose(WizardDialog.java:859)
	at org.eclipse.jface.wizard.WizardDialog.close(WizardDialog.java:484)
	at org.eclipse.jface.window.Window.handleShellCloseEvent(Window.java:741)
	at org.eclipse.jface.window.Window$3.shellClosed(Window.java:687)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:98)

Comment 1 Hitesh

2010-12-09 07:32:07 EST

For this and the other bug (Bug 331335) the following thought occurred to me:
There is a potential for exception almost, always when calling third party code. Now, guarding for exceptions at every single call, although highly defensive, seems like an overdo. 
If  on the other hand, exceptions occur due to the code flow of the framework itself, then it should certainly be addressed by the makers of the framework. Just wondering if there exist a situation like that here.

Comment 2 Remy Suen

2010-12-09 07:56:52 EST

(In reply to comment #1)
> There is a potential for exception almost, always when calling third party
> code. Now, guarding for exceptions at every single call, although highly
> defensive, seems like an overdo.

I disagree.

For example, listener notifications are pretty much always seen as being wrapped inside an ISafeRunnable to ensure that one renegade listener doesn't kill the whole notification loop as that would effectively cause the rest of the listeners to not be notified.

The same thing applies in this case. Take a look at the code in ViewReference's createPartHelper(). Both init(IViewSite, IMemento) and createPartControl(Composite) is wrapped in a try/catch. The dispose() call farther down the line is also protected and the same pattern can be seen in WorkbenchPartReference's doDisposePart() method. We protect ourselves from bad code in workbench parts, so we should also protect ourselves from bad code in wizards.

As I mentioned on bug 331335 comment 1, not allowing the wizard to be closed is a big problem as the user is now effectively "stuck" because it's blocking the workbench window. If one bad plug-in can force you to kill your Eclipse process, then we are doing something wrong.

Comment 3 Hitesh

2010-12-09 08:59:52 EST

(In reply to comment #2)

I am not going to discuss this any further, but my stand remains. As for the fix I leave it between Prakash and you.

Comment 4 Remy Suen

2010-12-09 09:15:54 EST

(In reply to comment #3)
> I am not going to discuss this any further, but my stand remains.

This is very unfortunate because I would much prefer we either a) both agree that this is important or b) both agree that this is unnecessary and we do not need to be overly cautious about client code.

Is your concern here over it being an ugly code pattern and makes the code more verbose and difficult to read? That it represents over engineering creep in the code base?

Or is your concern here over performance, that these try/catch blocks would slow things down? Or perhaps something entirely different?

----------

To put things into context, the reason I opened this bug is because I actually got into this state while testing EGit with 4.x and I was forced to terminate my Eclipse process via the Windows Task Manager.

Comment 5 Hitesh

2010-12-09 09:23:34 EST

(In reply to comment #4)
This one deserves one more comment from me ;) It is more about programming errors and runtime conditions, and yes ,of course, the style of such code is distasteful to say the least!!
Cheers,
Hitesh

Comment 6 Prakash Rangaraj

2010-12-10 00:46:18 EST

To guard or not to guard, depends on the behaviour caused by that exception in the third party code. If it results in an entry in the Error Log, then its acceptable. We don't have to guard that piece of code. But if it results in force shutdown of Eclipse, then it is completely unacceptable. We need to guard against the behaviour.

Targeting for M5

Comment 7 Prakash Rangaraj

2010-12-14 03:02:56 EST

Created attachment 185109 [details]
Patch v01

Comment 8 Prakash Rangaraj

2010-12-14 05:58:22 EST

Patch v01 released to HEAD

Comment 9 Prakash Rangaraj

2011-01-25 03:56:14 EST

Verified in I20110124-1800