tomcz/s3dropbox — Issue #1: Potential Swing thread access violation errors in s3dropbox
Author: saizhang
Created: 2012-01-12 22:39:42+00:00
State: open

Hi there-

I am writing an automated tool to find potential errors in software. I used it to check
the "single thread rule" in Swing applications, e.g., all GUI objects must be accessed
from the "event dispatch thread", and found there may be a few potential violations in
s3dropbox's code base.

Here is one example:

in class: com.tomczarniecki.s3.gui.DeleteBucketAction

private void deleteBucket() {
executor.execute(new Runnable() { //(1)
public void run() {
try {
controller.deleteCurrentBucket(); //(2)
} catch (Exception e) {
logger.info("Delete failed", e);
deleteError(); //(3)
}
}
});
}

private void deleteError() {
String text = "Cannot delete folder %s.\nPlease make sure that it is empty and try again.";
display.showErrorMessage("Delete failed", String.format(text, controller.getSelectedBucketName())); //(4)
}

(1) spawns a new thread, which aims to delete the current buckets at (2). However, it there is an exception the deleteError code at (3) will directly access UI object at (4).

Inside (4), the showErrorMessage method will eventually call: JOptionPane.showMessageDialog, which is not thread-safe in swing. In other words, that method should only be called via the event dispatch thread (EDT).

As indicated by Swing's documentation. Swing is a single-threaded UI toolkit. Because it is not thread-safe, all Swing code must be executed in the EDT. As the official documentation states, invoking Swing code from multiple threads risks thread interference or memory-consistency errors. In addition, Swing does not provide any run-time checks for correct EDT usage and, most of the time, you can get away with an apparently "well behaved" Swing UI that breaks these rules.

I also note other places in s3dropbox already take care of this rule, such as:
inside DropboxTableModel.java

private void updateView() {
worker.executeOnEventLoop(new Runnable() {
public void run() {
fireTableDataChanged();
}
});
}

It explicitly update the UI inside the event loop. So, technically, the error displaying code should also be executed inside the event loop (though the app may "behave" well now).

How do you think about such potential errors? Any comments, suggestion are highly appreciated; and will help us improve the tool.

I also have a number of reported errors, I am also willing to post them here.

Thanks a lot

-Sai