Bug 265741 - Cannot dispose clipboard in Activator stop()
Summary: Cannot dispose clipboard in Activator stop()
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 3.4.1   Edit
Hardware: PC Windows Vista
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Kevin Barnes CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-21 14:56 EST by Kenneth Evans, Jr. CLA
Modified: 2019-09-06 16:15 EDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Evans, Jr. CLA 2009-02-21 14:56:42 EST
Build ID: M20080911-1700

Steps To Reproduce:
1. Call dispose() in stop method of Activator
2. Throws SWTException
3.

More information:

I get an SWTException trying to dispose the clipboard in an Activator stop method.  The clipboard is _not_ disposed at the time.  The code in Clipboard.java that causes the exception is:

	if (display.getThread() != Thread.currentThread()) DND.error(SWT.ERROR_THREAD_INVALID_ACCESS);

Display.getCurrent() is null at the time.  The problem thus seems to be that the display is gone, leaving the clipboard undisposed.

This may not be a real problem in that we are shutting down, but it does seem as though we should be able to dispose the clipboard anyway or that it should be disposed for us when the Display goes away.

This my code:

	public void stop(BundleContext context) throws Exception {
		try {
                        ...
			// This gets an exception and doesn't really do anything.
			try {
				System.out.println("cur=" + Thread.currentThread());
				System.out.println("display=" + Display.getCurrent());
				if (FableClipboard.clipboard != null
						&& !FableClipboard.clipboard.isDisposed()) {
					FableClipboard.clipboard.dispose();
				}
			} catch (SWTException ex) {
				// This is the exception it gets, indicating the device
				// (presumably the display) is disposed.
			} catch (Throwable t) {
				// Just to be sure ;-) Do nothing.
			}
		} finally {
			super.stop(context);
		}
	}
Comment 1 Remy Suen CLA 2009-02-21 15:09:18 EST
Is your Activator a subclass of AbstractUIPlugin? If yes, I would technically imagine that the stop(BundleContext) method would only be called in a UI thread. Also, perhaps you could instantiate new Clipboard instances and dispose them when you're done instead of having a global one in the activator?

Anyway, if for whatever reasons you have to have this global one, you could consider using Display's disposeExec(Runnable) method and have your clipboard disposed when the display is disposed?
Comment 2 Kenneth Evans, Jr. CLA 2009-02-21 16:27:28 EST
Thanks for the reply.

1. It wasn't clear from the documentation (at least that I found) that it was Ok to dispose the clipboard when there is something in it.  If you can do that, then I don't need a global one and can just dispose it each time after adding something.  Is that OK?

2. Yes, there are probably ways to work around this, for example, the disposeExec, if it works as you say, or some sort of dispose listener.  However, it seems the logic that is keeping you from disposing the Clipboard is not correct.  Whatever native resources it has allocated could still be freed.

3. Yes, the Activator extends AbstractUIPlugin.  I couldn't check the thread.  Display.getCurrent() is null in the stop() method, though apparently not the display in Clipboard.java.  The clipboard was started with Display.getCurrent().  Using Display.getDefault() there gives the same problem.  I want it to be final so must get set when the class is created.  (Not sure when that happens owing to lazy loading).

4.  The reason for the exception says "device is disposed", _not_ that we are in the wrong thread.

This is the FableClipboard class:

final public class FableClipboard {
	public static final Clipboard clipboard = new Clipboard(Display
			.getDefault());
}


BTW the Javadoc for the Activator stop method talks about shutdown, rather than stop, which if not wrong, is unclear.

Thanks again,

-Ken


Comment 3 Remy Suen CLA 2009-02-21 23:19:48 EST
(In reply to comment #2)
> 1. It wasn't clear from the documentation (at least that I found) that it was
> Ok to dispose the clipboard when there is something in it.  If you can do that,
> then I don't need a global one and can just dispose it each time after adding
> something.  Is that OK?

Well, according to the APIs, it's still going to be on the clipboard after it's been disposed but may not be if the application itself has been shut down.

"Disposes of the operating system resources associated with the clipboard. The data will still be available on the system clipboard after the dispose method is called.

NOTE: On some platforms the data will not be available once the application has exited or the display has been disposed."

http://help.eclipse.org/stable/nftopic/org.eclipse.platform.doc.isv/reference/api/org/eclipse/swt/dnd/Clipboard.html#dispose()

> This is the FableClipboard class:
> 
> final public class FableClipboard {
>         public static final Clipboard clipboard = new Clipboard(Display
>                         .getDefault());
> }

Where do you reference this FableClipboard class anyway? Instead of calling Display.getDefault(), I would strongly recommend you go through your AbstractUIPlugin instead with something like MyPlugin.getDefault().getWorkbench().getDisplay().

> BTW the Javadoc for the Activator stop method talks about shutdown, rather than
> stop, which if not wrong, is unclear.

If the javadocs is unclear about something, then I suggest you file a separate bug against Platform/UI for clarifications.
Comment 4 Kenneth Evans, Jr. CLA 2009-02-22 00:38:54 EST
I've looked at this more since submitting the bug.  Almost all of the examples, snipppet94, for example have a global clipboard.  I am new at using the clipboard, so that is what I did.  I rewrote the code to be:

ImageTransfer transfer = ImageTransfer.getInstance();
Clipboard clipboard = new Clipboard(getDisplay());
clipboard.setContents(new Object[] { getImageData() },
  new Transfer[] { transfer });
clipboard.dispose();

That is, make a new Clipboard and dispose of it immediately.  This seems to work, though that is not what the examples do.  They have a global clipboard.  It eliminates the problem, though, and doesn't seem to be a bad thing to do.

What I had before, in answer to your question is:

ImageTransfer transfer = ImageTransfer.getInstance();
FableClipboard.clipboard.setContents(new Object[] { getImageData() },
  new Transfer[] { transfer });

which also works, but has the dispose problem.  This way has a global clipboard like the examples.

In regard to the display: there is only one Display in this application (as for most applications) so it should not matter where I create the global clipboard.  The argument will always be the same Display object.  There is only one.

The problem is that you can't dispose it if the Display has been disposed.  It is only necessary to check the thread if there is more than one Display, since they have to run in separate threads.  The reason for the check that gives the SWTException) is just to be sure you have the correct Display.  It is failing, not for this reason, but rather because there is _no_ Display  (It would, in fact, be the right thread, since there is only one Display thread in this application, as in most applications).

I would guess the resources could be disposed even though the Display is gone if the logic were rethought.  Maybe it needs to look at the thread _and_ at whether the Display it has stored is disposed.

I can't get any further as the debugger is not using the same source as the class, and I can't trace through the logic.  The lines don't match.  In any event, I think someone more knowledgeable should take a look at it.

It seems to be that one will run into this problem if he tries to have a global clipboard in an RCP application (and tries to dispose of it as one is taught to do ;-).

Thanks again for your help.
Comment 5 Remy Suen CLA 2009-02-22 07:06:16 EST
(In reply to comment #4)
> In regard to the display: there is only one Display in this application (as for
> most applications) so it should not matter where I create the global clipboard.
>  The argument will always be the same Display object.  There is only one.

Using Display.getDefault() during bundle activation is dangerous per bug 250048. It is not a good idea to assume that there is only one Display instance in an application.

As to the rest of your questions, I'll withhold from commenting so someone from the SWT team can address them.
Comment 6 Felipe Heidrich CLA 2009-02-23 10:31:45 EST
Can you reproduce this problem in a SWT only example ?

IMO, this problem is not SWT. If I understood correctly,  Kenneth wants to know when a RPC app/plugin should dispose the clipboard.
Comment 7 Remy Suen CLA 2009-02-23 10:36:12 EST
(In reply to comment #6)
> IMO, this problem is not SWT. If I understood correctly,  Kenneth wants to know
> when a RPC app/plugin should dispose the clipboard.

To me, I think Kenneth also seems to wonder why one can't dispose a Clipboard after its parent Display has been disposed.

(In reply to comment #4)
> The problem is that you can't dispose it if the Display has been disposed.  It
> is only necessary to check the thread if there is more than one Display, since
> they have to run in separate threads.  The reason for the check that gives the
> SWTException) is just to be sure you have the correct Display.  It is failing,
> not for this reason, but rather because there is _no_ Display  (It would, in
> fact, be the right thread, since there is only one Display thread in this
> application, as in most applications).
> 
> I would guess the resources could be disposed even though the Display is gone
> if the logic were rethought.  Maybe it needs to look at the thread _and_ at
> whether the Display it has stored is disposed.
Comment 8 Steve Northover CLA 2009-02-23 10:55:37 EST
If the display is disposed, then dispose() fails silently (actually, it doesn't fail because the underlying windows system has been exited and resources associated with it are given back).

Kevin, please fix Clipboard (and Drag and Drop) to do this.
Comment 9 Felipe Heidrich CLA 2009-02-23 11:06:49 EST
I see, so here is the snippet:
public static void main (String[] args) {
        Display  display = new Display ();
        Clipboard clipboard = new Clipboard(display);
        display.dispose ();
        clipboard.dispose();
}
And here is the stack:
Exception in thread "main" org.eclipse.swt.SWTException: Device is disposed
        at org.eclipse.swt.SWT.error(SWT.java:3860)
        at org.eclipse.swt.SWT.error(SWT.java:3775)
        at org.eclipse.swt.SWT.error(SWT.java:3746)
        at org.eclipse.swt.widgets.Display.error(Display.java:1186)
        at org.eclipse.swt.widgets.Display.getThread(Display.java:2430)
        at org.eclipse.swt.dnd.Clipboard.dispose(Clipboard.java:213)
        at misc.prs.PR.main(PR.java:13)

=-=-=-=-=-=-=-=-=-=-=
Steve, this is something we might want to change.

that said, the application should dispose the display last!
Comment 10 Kenneth Evans, Jr. CLA 2009-02-23 11:39:04 EST
>> IMO, this problem is not SWT. If I understood correctly,  Kenneth wants to know when a RPC app/plugin should dispose the clipboard.

Well, I now know.  In an RCP app, however, the most obvious place to do this is in an Activator stop method.  It is too late then, because the Display has been disposed, but not by you.  So unlike the the simple snippet, it isn't so easy to know when to dispose the clipboard.  Probably using Display.disposeExec (as suggested by Remy) is a good way.  The problem is none of this is documented and you have to figure it out by trial and error.

>> If the display is disposed, then dispose() fails silently

No, it throws an SWTException.  Otherwise I would not have noticed.  But probably it _should_ fail silently, as long as the resources _have_ been freed correctly.

>> that said, the application should dispose the display last!

Yes, but the problem is, as mentioned above, that in an RCP app, someone else disposes the clipboard and apparently before Activator.stop() is called, so it is difficult to know how to do that.

>> Kevin, please fix Clipboard (and Drag and Drop) to do this.

Thanks.  IMHO it seems that is what needs to be done.

I did change my application to construct a new Clipboard, do the copy, then dispose it immediately as mentioned.  This seems to work, but is not what the examples and snippets indicate.  Some documentation would help.  It is not clear it is OK to do this.

Actually IMHO the Clipboard Javadoc should mention you have to dispose the clipboard _before_ the display is disposed. (Easy fix)

Thanks to everyone for the help.

-Ken



Comment 11 Remy Suen CLA 2009-02-23 11:53:18 EST
(In reply to comment #10)
> I did change my application to construct a new Clipboard, do the copy, then
> dispose it immediately as mentioned.  This seems to work, but is not what the
> examples and snippets indicate.

I should note that when I posed the suggestion of not creating a global Clipboard instance I didn't necessarily mean to do something like the following.

Clipboard cb = new Clipboard(display);
cb.setContents(/* ... */);
cb.dispose();

What I meant to suggest was to have a Clipboard per "set of user interface widgets". Like if you needed to do some copy/pasting in a view of your RCP application, then you'd just instantiate a new Clipboard when your view's createPartControl(Composite) method is called and subsequently dispose it in your implementation of the dispose() method (and of course, you should remember to call super.dispose()). The SWT snippets are essentially self-contained "applications", similar to an RCP view, which is why I think they have a "global" Clipboard.

> Some documentation would help.  It is not
> clear it is OK to do this.

I guess it may be worth noting in the class-level javadocs instead of simply in the documentation for Clipboard's dispose() method.
Comment 12 Kenneth Evans, Jr. CLA 2009-02-23 12:18:18 EST
>> I didn't necessarily mean to do something like the
following.

Clipboard cb = new Clipboard(display);
cb.setContents(/* ... */);
cb.dispose();

So, is it bad to do this?  It makes it encapsulated, which is good.  For a copy operation speed is not that important, and I would guess the cost is minor anyway.

The problem with the RCP application I am working on is that it is large and has many plug-ins and views that interact, so my "set of user interface widgets" is most of the application.  Thus the global clipboard I had first (and could still use now that I know how to dispose it without getting exceptions) seems to be the way to go _if_ you need to go global.  I don't see that you do.  I have only tested on Windows so far, though.

Thanks,

-Ken

Comment 13 Remy Suen CLA 2009-02-23 12:23:15 EST
(In reply to comment #12)
> So, is it bad to do this?

I was trying to put things into the context of the examples you might have seen (and a possible explanation as to why they don't do it your way). I have seen code like yours before too though.
Comment 14 Eclipse Webmaster CLA 2019-09-06 16:15:28 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.