Bug 343721

Description William Chen 2011-04-25 04:47:25 EDT

Build Identifier: 20100917-0705

This seems to be a regression created by a code refactoring. Find the code in the plugin org.eclipse.ui.navigator (version 3.5.0.I20100601-0800). In the method getParent of org.eclipse.ui.internal.navigator.NavigatorContentServiceContentProvider, the code is changed to use SafeRunner.run to find the parent. However, there's an error in the code:
	public Object getParent(final Object anElement) {
		final Set extensions = contentService.findContentExtensionsWithPossibleChild(anElement);
		final Object[] parent = new Object[1];

		for (Iterator itr = extensions.iterator(); itr.hasNext();) {
			final NavigatorContentExtension foundExtension = (NavigatorContentExtension) itr.next();

			SafeRunner.run(new NavigatorSafeRunnable() {
				NavigatorContentExtension[] overridingExtensions;

				public void run() throws Exception {
					if (!isOverridingExtensionInSet(foundExtension.getDescriptor(), extensions)) {
						parent[0] = foundExtension.internalGetContentProvider()
								.getParent(anElement);
						overridingExtensions = foundExtension
								.getOverridingExtensionsForPossibleChild(anElement);
						if (overridingExtensions.length > 0) {
							parent[0] = pipelineParent(anElement, overridingExtensions, parent);
						}

						if (parent[0] != null) {
							return;
						}
					}
				}

				public void handleException(Throwable e) {
					NavigatorPlugin.logError(0, NLS.bind(
							CommonNavigatorMessages.Exception_Invoking_Extension, new Object[] {
									foundExtension.getDescriptor().getId(), anElement }), e);
				}
			});
		}
		return parent[0];
	}
Pay attention to the above three lines:
if (parent[0] != null) {
     return;
}
They are meaningless since they are at the end of the method run. I checked a previous version of this class. The corresponding method is written like:
	public synchronized Object getParent(Object anElement) {
		Set extensions = contentService
				.findContentExtensionsWithPossibleChild(anElement);

		Object parent;
		NavigatorContentExtension foundExtension;
		NavigatorContentExtension[] overridingExtensions;
		for (Iterator itr = extensions.iterator(); itr.hasNext();) {
			foundExtension = (NavigatorContentExtension) itr.next();
			try {

				if (!isOverridingExtensionInSet(foundExtension.getDescriptor(),
						extensions)) {

					parent = foundExtension.internalGetContentProvider().getParent(
							anElement);

					overridingExtensions = foundExtension
							.getOverridingExtensionsForPossibleChild(anElement);

					if (overridingExtensions.length > 0) {
						parent = pipelineParent(anElement,
								overridingExtensions, parent);
					}

					if (parent != null) {
						return parent;
					}
				}
			} catch (RuntimeException re) {
				NavigatorPlugin
						.logError(
								0,
								NLS
										.bind(
												CommonNavigatorMessages.Could_not_provide_children_for_element,
												new Object[] { foundExtension
														.getDescriptor()
														.getId() }), re);
			} catch (Error e) {
				NavigatorPlugin
						.logError(
								0,
								NLS
										.bind(
												CommonNavigatorMessages.Could_not_provide_children_for_element,
												new Object[] { foundExtension
														.getDescriptor()
														.getId() }), e);

			}
		}

		return null;
	}
 It is clear that getParent was once refactored to use SafeRunner.run to safe-guard the code. Note the corresponding three lines reads like:
if(parent!=null){
      return parent.
}
When this code is refactored, it is put in the wrong place, i.e., in the run method of SafeRunner, which causes a wrong result. The code does not search for the first non-null parent at all. It goes through all the extensions and returns the final one. The code should be like the following to keep the original searching order:
	public Object getParent(final Object anElement) {
		final Set extensions = contentService.findContentExtensionsWithPossibleChild(anElement);
		final Object[] parent = new Object[1];

		for (Iterator itr = extensions.iterator(); itr.hasNext();) {
			final NavigatorContentExtension foundExtension = (NavigatorContentExtension) itr.next();

			SafeRunner.run(new NavigatorSafeRunnable() {
				NavigatorContentExtension[] overridingExtensions;

				public void run() throws Exception {
					if (!isOverridingExtensionInSet(foundExtension.getDescriptor(), extensions)) {
						parent[0] = foundExtension.internalGetContentProvider()
								.getParent(anElement);
						overridingExtensions = foundExtension
								.getOverridingExtensionsForPossibleChild(anElement);
						if (overridingExtensions.length > 0) {
							parent[0] = pipelineParent(anElement, overridingExtensions, parent);
						}

					}
				}

				public void handleException(Throwable e) {
					NavigatorPlugin.logError(0, NLS.bind(
							CommonNavigatorMessages.Exception_Invoking_Extension, new Object[] {
									foundExtension.getDescriptor().getId(), anElement }), e);
				}
			});
			if (parent[0] != null) {
				return;
			}
		}
		return parent[0];
	}
That is, move the three lines out of run and put it after SafeRunner.run call.

We found this in our own IDE. The result is that the navigator buttons in the toolbar of Project Explorer do not work properly any more. In our case, the call getParent over a project does not return the workspace root. It returns the project itself. So the toolbar button "Up to parent" does not work any more.



Reproducible: Always