Comment 1 Markus Keller 2009-04-28 05:51:33 EDT

I can reproduce in I20090427-1800 Cocoa with these steps:
1. change a line in a MANIFEST.MF
2. leave the MANIFEST.MF editor open (otherwise I run into bug 271393)
3. in the Synchronize view, open the outgoing change
4. close the compare editor with Command+W
5. repeat 3 & 4 quickly. After about 3 iteration, I usually get this NPE.

Comment 2 Tomasz Zarna 2009-04-28 07:06:05 EDT

I've just seen it too, thanks for the steps Markus.

Comment 3 Tomasz Zarna 2009-04-28 10:23:07 EDT

I'm not able to reproduce it on my single core machine, but I have no problem in seeing it on a dual core one. This could indicate a timing issue between when Compare Editor is disposed (in reaction to closing the editor either by user or self-closing on "Override and Update") and when the merge viewer is updated.

Comment 4 Dani Megert 2009-04-29 03:42:48 EDT

This should get 3.5 target.

Comment 5 Tomasz Zarna 2009-04-29 08:37:49 EDT

Apparently, we're able to dispose the Compare Editor with its viewers between org.eclipse.compare.CompareViewerSwitchingPane line 270 where we set input on the viewer and line 275 when we try to set an image on the same viewer. The problem is that the viewer can be disposed in the meantime. This is possible because CVSP in line 270 calls org.eclipse.jface.viewers.ContentViewer.setInput(Object) which in turn uses input hook org.eclipse.compare.contentmergeviewer.ContentMergeViewer.inputChanged(Object, Object) which schedules diffs recalculation (among other things, like updating the UI). I assume that processing org.eclipse.compare.internal.merge.DocumentMerger.doDiff() can take a while and this is the time frame within which we are able to close and dispose the editor. 

Comment 6 Tomasz Zarna 2009-04-29 09:18:28 EDT

I guess this wouldn't happen if we didn't schedule to process diffs in a separate thread (DocumentMerger line 435).

Comment 7 Tomasz Zarna 2009-04-29 09:43:59 EDT

Created attachment 133762 [details]
Possible fix

We already check widgets if "they are ok to use" [1] while updating the UI after diffs have been recalculated[2], so a possible fix here could be to add a similar check in CompareViewerSwitchingPane.setInput(Object) just after line containing "fViewer.setInput(input);"[3].

[1] org.eclipse.compare.internal.Utilities.okToUse(Widget)
[2] TextMergeViewer.update(boolean)
[3] org.eclipse.compare.CompareViewerSwitchingPane.setInput(Object) line 270

Comment 8 Tomasz Zarna 2009-04-29 09:44:03 EDT

Created attachment 133763 [details]
mylyn/context/zip

Comment 9 Tomasz Zarna 2009-04-29 11:59:36 EDT

Created attachment 133784 [details]
Patch v02

Comment 10 Tomasz Zarna 2009-04-29 12:58:25 EDT

Patch v02 adds a lot of checks and eventually exposes an API contract breakage for CompareEditorInput.createContents(Composite) ie there are cases when it returns null for the created SWT control...

Comment 11 Tomasz Zarna 2009-04-30 05:12:11 EDT

I did a quick check and it appears to be a regression introduced in 3.5, it doesn't show up in 3.4. I would like to investigate the problem further, get closer to the real cause and hopefully propose a better fix than adding multiple checks as in Patch v02.

Comment 12 Szymon Brandys 2009-04-30 09:52:11 EDT

(In reply to comment #11)
Just did some checks. It seems that the code mentioned in comment 6 behaves differently on Carbon and Cocoa. The issue is easily reproducible on Cocoa M6 with steps from comment 1, however Carbon version works fine without the exception. 

Comment 13 Tomasz Zarna 2009-05-05 06:49:09 EDT

Confirming what Szymon found out in the previous comment, the problem is reproducible on M4 Cocoa as well. Unfortunately, I'm not able to check whether this occurs on earlier builds, M4 is the earliest build supporting Cocoa and the iMac we did the tests on is the only machine with dual core CPU we have over here. Anyway, this appears to be an issue introduced some time ago, potentially even prior 3.5.

Comment 14 Dani Megert 2009-05-05 06:52:12 EDT

Please note that the bug was originally reported against WindowsXP, so it is not too important whether a cocoa build was available or not.

Comment 15 Tomasz Zarna 2009-05-05 09:02:11 EDT

(In reply to comment #14)
> Please note that the bug was originally reported against WindowsXP, so it is not too important whether a cocoa build was available or not.

Right, but having a dual core box seems to be relevant and the iMac is the only machine available locally that meets this req. I'm not saying it's a problem with Cocoa only. However, it would be helpful if we knew that it actually happened on WindowsXP. John?

Comment 16 John Arthorne 2009-05-05 09:04:25 EDT

> However, it would be helpful if we knew that it actually happened on WindowsXP. John?

Yes, it actually happened on XP. I'm pretty sure I'm not running a Mac ;)

Comment 17 Tomasz Zarna 2009-05-06 06:05:11 EDT

Found bug 275123 when trying to reproduce this issue on Windows using almost identical steps given by Markus in comment 1.

Comment 18 Tomasz Zarna 2009-05-06 06:06:49 EDT

Szymon could you review the second patch? I'm especially interested in your opinion about the issue mentioned in comment 10.

Comment 19 Tomasz Zarna 2009-05-12 07:40:18 EDT

Szymons seems to be quite busy with other tasks. Pawel, could you review the latest patch instead of him?

Comment 20 Pawel Pogorzelski 2009-05-13 03:16:34 EDT

Created attachment 135519 [details]
Patch_v03

Patch_v03 with a minor change.

Comment 21 Tomasz Zarna 2009-05-13 07:07:58 EDT

There is still one question remaining: are we allowed to return null instead of an SWT control from CompareEditorInput.createContents(Composite)? Despite the fact that this would happen only in certain, rare situations this is still a breaking change. What is worth mentioning is that without the fix a client will receive, in the same situation, an NPE (or "Widget is disposed" error) thrown from Compare. A proper solution here would be to change the contract for the createContents method, so it now may return null when Compare editor has been closed in the meantime. Moreover, this would have to be mentioned in Migration Guide. Given that I'm not sure is 3.5 better with the fix or without it. What do you think?

Comment 22 Szymon Brandys 2009-05-13 07:41:30 EDT

(In reply to comment #21)
> There is still one question remaining: are we allowed to return null instead of
> an SWT control from CompareEditorInput.createContents(Composite)? Despite the
> fact that this would happen only in certain, rare situations this is still a
> breaking change. What is worth mentioning is that without the fix a client will
> receive, in the same situation, an NPE (or "Widget is disposed" error) thrown
> from Compare. 

I can imagine that some clients being aware of that handle those exceptions in their code. Thus changing the contract for returned parameter would be breaking change for them.

We can't say right now whether there are any other clients affected by this change and there won't be much time to figure it out.

Going to the suggested fix. Can't we just catch those NPEs and "Widget is disposed" wherever we call CompareEditorInput.createContents(Composite) for 3.5 and give a better fix for 3.5.1?

I'm still not sure when this was introduced. It seems that the issue has been there for a while (prior 3.5), however was exposed in Cocoa version (see comment 12). If this is true, the issue should be reproducible on Win, dual core using 3.4. 

Comment 23 Tomasz Zarna 2009-05-14 08:35:44 EDT

I've just tried to reproduce it using 3.5M7 on a T61 equipped in a dual-core CPU (Core 2 Duo) with no success. John, what kind of CPU do you have on your box?

Comment 24 John Arthorne 2009-05-14 08:50:28 EDT

That is the same as what I have - core 2 duo on T61. Note that I only ever reproduced this once, so it wouldn't surprise me that it's hard to reproduce.

Comment 25 Tomasz Zarna 2009-05-14 11:04:15 EDT

We talked to Dani and decided to defer it to 3.5.1. This way we will gain time to investigate and look for a better fix (without breaking the API). Moreover, if there won't be any other similar bugs (dupes) reported we could even consider this bug for 3.6 only.

Comment 26 Tomasz Zarna 2009-08-20 11:16:59 EDT

Here is a similar exception I caught today when I closed Eclipse just after I asked it to open a bunch of Compare Editors from the Sync View:

Daemon Thread [Thread-0] (Suspended (exception NullPointerException))	
	CompareContentViewerSwitchingPane.setImage(Image) line: 227	
	CompareContentViewerSwitchingPane(CompareViewerSwitchingPane).setInput(Object) line: 275	
	CompareContentViewerSwitchingPane.setInput(Object) line: 132	
	ModelCompareEditorInput(CompareEditorInput).internalSetContentPaneInput(Object) line: 812	
	CompareEditorInput.access$7(CompareEditorInput, Object) line: 810	
	CompareEditorInput$10.run() line: 750	
	BusyIndicator.showWhile(Display, Runnable) line: 70	
	ModelCompareEditorInput(CompareEditorInput).feed1(ISelection) line: 744	
	ModelCompareEditorInput(CompareEditorInput).feedInput() line: 722	
	ModelCompareEditorInput(CompareEditorInput).createContents(Composite) line: 545	
	CompareEditor.createCompareControl() line: 449	
	CompareEditor.access$6(CompareEditor) line: 416	
	CompareEditor$3.run() line: 372	
	UILockListener.doPendingWork() line: 155	
	UISynchronizer$3.run() line: 158	
	RunnableLock.run() line: 35	
	UISynchronizer(Synchronizer).runAsyncMessages(boolean) line: 134	
	Display.runAsyncMessages(boolean) line: 3442	
	Display.readAndDispatch() line: 3139	
	ModalContext$ModalContextThread.block() line: 173	
	ModalContext.run(IRunnableWithProgress, boolean, IProgressMonitor, Display) line: 388	
	ProgressMonitorJobsDialog(ProgressMonitorDialog).run(boolean, boolean, IRunnableWithProgress) line: 507	
	ProgressMonitorJobsDialog.run(boolean, boolean, IRunnableWithProgress) line: 275	
	ProgressManager$4.run() line: 966	
	BusyIndicator.showWhile(Display, Runnable) line: 70	
	ProgressManager.busyCursorWhile(Runnable, ProgressMonitorJobsDialog) line: 1001	
	ProgressManager.busyCursorWhile(IRunnableWithProgress) line: 976	
	ProgressManager.run(boolean, boolean, IRunnableWithProgress) line: 1182	
	OpenInCompareAction.isOkToOpen(ISynchronizePageSite, ISynchronizeParticipant, ICompareInput) line: 124	
	OpenInCompareAction.openCompareEditor(ISynchronizePageConfiguration, Object, boolean, boolean) line: 112	
	OpenInCompareAction.run() line: 65	
	OpenInCompareAction(Action).runWithEvent(Event) line: 498	
	ActionContributionItem.handleWidgetSelection(Event, boolean) line: 584	
	ActionContributionItem.access$2(ActionContributionItem, Event, boolean) line: 501	
	ActionContributionItem$5.handleEvent(Event) line: 411	
	EventTable.sendEvent(Event) line: 84	
	Display.sendEvent(EventTable, Event) line: 3580	
	MenuItem(Widget).sendEvent(Event) line: 1259	
	MenuItem(Widget).sendEvent(int, Event, boolean) line: 1282	
	MenuItem(Widget).sendEvent(int, Event) line: 1267	
	MenuItem(Widget).notifyListeners(int, Event) line: 1088	
	Display.runDeferredEvents() line: 3478	
	Display.readAndDispatch() line: 3137	
	Workbench.runEventLoop(Window$IExceptionHandler, Display) line: 2405	
	Workbench.runUI() line: 2369	
	Workbench.access$4(Workbench) line: 2221	
	Workbench$5.run() line: 500	
	Realm.runWithDefault(Realm, Runnable) line: 332	
	Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 493	
	PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 149	
	IDEApplication.start(IApplicationContext) line: 113	
	EclipseAppHandle.run(Object) line: 194	
	EclipseAppLauncher.runApplication(Object) line: 110	
	EclipseAppLauncher.start(Object) line: 79	
	EclipseStarter.run(Object) line: 367	
	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: 585	
	Main.invokeFramework(String[], URL[]) line: 610	
	Main.basicRun(String[]) line: 565	
	Main.run(String[]) line: 1362	
	Main.main(String[]) line: 1338	

Comment 27 Tomasz Zarna 2009-08-20 11:28:44 EDT

(In reply to comment #26)
> Here is a similar exception I caught today when I closed Eclipse just after I
> asked it to open a bunch of Compare Editors from the Sync View:

Another exception I get from time to time with the same steps:

java.lang.NullPointerException
at org.eclipse.ui.internal.EditorManager.checkCreatePinEditorShortcutKeyHandler(EditorManager.java:245)
at org.eclipse.ui.internal.EditorReference.initListenersAndHandlers(EditorReference.java:237)
at org.eclipse.ui.internal.EditorReference.<init>(EditorReference.java:125)
at org.eclipse.ui.internal.EditorManager.openEditorFromDescriptor(EditorManager.java:651)
at org.eclipse.ui.internal.EditorManager.openEditor(EditorManager.java:638)
at org.eclipse.ui.internal.WorkbenchPage.busyOpenEditorBatched(WorkbenchPage.java:2854)
at org.eclipse.ui.internal.WorkbenchPage.busyOpenEditor(WorkbenchPage.java:2762)
at org.eclipse.ui.internal.WorkbenchPage.access$11(WorkbenchPage.java:2754)
at org.eclipse.ui.internal.WorkbenchPage$10.run(WorkbenchPage.java:2705)
at org.eclipse.swt.custom.BusyIndicator.showWhile(BusyIndicator.java:70)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:2701)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:2685)
at org.eclipse.ui.internal.WorkbenchPage.openEditor(WorkbenchPage.java:2676)
at org.eclipse.compare.internal.CompareUIPlugin$1.run(CompareUIPlugin.java:518)
at org.eclipse.compare.internal.CompareUIPlugin.syncExec(CompareUIPlugin.java:1258)
at org.eclipse.compare.internal.CompareUIPlugin.internalOpenEditor(CompareUIPlugin.java:529)
at org.eclipse.compare.internal.CompareUIPlugin.openEditorInBackground(CompareUIPlugin.java:499)
at org.eclipse.compare.internal.CompareUIPlugin.openCompareEditor(CompareUIPlugin.java:488)
at org.eclipse.compare.CompareUI.openCompareEditorOnPage(CompareUI.java:168)
at org.eclipse.compare.CompareUI.openCompareEditorOnPage(CompareUI.java:149)
at org.eclipse.team.internal.ui.synchronize.actions.OpenInCompareAction.openCompareEditor(OpenInCompareAction.java:209)
at org.eclipse.team.internal.ui.synchronize.actions.OpenInCompareAction.openCompareEditor(OpenInCompareAction.java:169)
at org.eclipse.team.internal.ui.synchronize.actions.OpenInCompareAction.openCompareEditor(OpenInCompareAction.java:113)
at org.eclipse.team.internal.ui.synchronize.actions.OpenInCompareAction.run(OpenInCompareAction.java:65)
at org.eclipse.jface.action.Action.runWithEvent(Action.java:498)
at org.eclipse.jface.action.ActionContributionItem.handleWidgetSelection(ActionContributionItem.java:584)
at org.eclipse.jface.action.ActionContributionItem.access$2(ActionContributionItem.java:501)
at org.eclipse.jface.action.ActionContributionItem$5.handleEvent(ActionContributionItem.java:411)
at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84)
at org.eclipse.swt.widgets.Display.sendEvent(Display.java:3580)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1259)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1282)
at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1267)
at org.eclipse.swt.widgets.Widget.notifyListeners(Widget.java:1088)
at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:3478)
at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3137)
at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2405)
at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2369)
at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2221)
at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:500)
at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:493)
at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:113)
at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:194)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:367)
at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
at java.lang.reflect.Method.invoke(Method.java:585)
at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:610)
at org.eclipse.equinox.launcher.Main.basicRun(Main.java:565)
at org.eclipse.equinox.launcher.Main.run(Main.java:1362)
at org.eclipse.equinox.launcher.Main.main(Main.java:1338)

Comment 28 Tomasz Zarna 2009-08-24 08:19:39 EDT

I've just managed to get a similar effect on 3.4.2[1], first I got the exception from bug 260894 (which is now fixed and replaced with the one from comment 0), a moment later I got the same exception as in comment 26. I believe this proves what Szymon suspected in comment 22 i.e. that this is not a regression introduced in 3.5 (unless we consider bug 221583 as the one causing the regression). 

Given that and the fact that the current fix would require a short note in the migration guide (breaking the contract, see comment 21) and keeping in mind that the exception occurs in rare situations (only dual core machines) we talked to McQ and decided *not* to backport it to 3.5.x stream. 

Hopefully we will find some time to find a better, non-breaking fix for this issue as well as the real cause of it. If found we could consider it for backporting to 3.5.2, otherwise we will probably stick to the current solution and add a note to the migration guide.

[1] eclipse-SDK-3.4.2-macosx-carbon

Comment 29 John Arthorne 2009-10-20 15:05:17 EDT

This is just a note that I encountered this NPE again today using I20091013-1302. Was the above patch ever released in 3.6?

Comment 30 Tomasz Zarna 2009-10-22 05:46:45 EDT

(In reply to comment #29)
> Was the above patch ever released in 3.6?
No, for pretty much the same reason as we decided not to backport it to 3.5.x. We would like to propose a better, non-breaking fix for this issue. Unfortunately, we haven't managed to do that so far, working on other tasks. But if you asked me I would go with the current fix, at least for 3.6.

Comment 31 Tomasz Zarna 2010-02-23 08:49:26 EST

Created attachment 159919 [details]
Patch v04

Another approach we could consider here is handling the "Widget is disposed" error.

Comment 32 Szymon Brandys 2010-02-25 04:51:07 EST

Catching 'widget is disposed' is the simplest and quite good solution here. 

Other acceptable solutions are:

1) #openCompareDialog should compute differences and show Compare Dialog sequentially in the same thread. This method would have to be run in the UI thread. A disadvantage of this solution is that the dialog would appear when computing is done, not immediately as it is done now.

2) Compare Dialog should listen for events/properties coming while differences are being computed. Then the dialog should decide how to refresh itself when computing is done.

Both approached mentioned above require Compare Dialog refactoring, what is no possible at this point of 3.6.

Comment 33 Szymon Brandys 2010-02-25 05:34:03 EST

(In reply to comment #32)
Writing 'Compare Dialog', I of course meant 'Compare Editor' ;) Sorry for confusion.

Comment 34 Tomasz Zarna 2010-03-04 11:30:25 EST

The latest patch has been applied to HEAD. Available in builds >N20100303-2000. 

In the meantime I filed bug 304693 to investigate how to improve the editor and make it more multi-threading friendly.

Comment 35 Tomasz Zarna 2010-03-11 06:39:04 EST

Guys, could any of you verify the fix? Thanks in advance.