BasicDirectoryModel getDirectories and DoChangeContents.run can deadlock

Report by Adrian Nistor <adrnisto@amazon.com>.

The code in BasicDirectoryModel.java acquires synchronization objects in different orders. One acquires DoChangeContents.this and then fileCache, the other acquires fileCache and then DoChangeContents.this.

Acquiring synchronization objects in different orders can cause a circular wait (deadlock).

Here is the first order: (1) first acquire DoChangeContents.this and then fileCache:

    enter public method run() at line 524
    acquire synchronization on DoChangeContents.this at entry to run() because run() is a synchronized method --- see line 524
    acquire synchronization on fileCache at line 528

Here is the second order: (2) first acquire fileCache and then DoChangeContents.this:

    enter public method renameFile() at line 176
    acquire synchronization on fileCache at line 177
    call validateFileCache() at line 179
    call cancelRunnables() at line 157
    call cancelRunnables(Vector) at line 397
    call cancel() at line 392
    acquire synchronization on DoChangeContents.this at entry to cancel() because cancel() is a synchronized method --- see line 520

A straight-forward fix always acquires synchronization in only one order: first fileCache and then DoChangeContents.this. Swap the order of synchronized (this) and synchronized (fileCache) at lines 524 and 528.

Removing the synchronized keyword from the method signature and replacing it with synchronized (this) ensures the same behavior.

public void run() { <<<<<<<<<<<<<<<<<<<<<<<<<<<<< removed synchronized from here and moved it to synchronized (this) 2 lines below
    synchronized(fileCache) { <<<<<<<<<<<<<<<<<<<<<<< added this statement instead of the same statement 5 lines below
        synchronized(this) { <<<<<<<<<<<<<<<<<<<<< added this instead of the synchronized keyword in the method signature 2 lines above
            if (fetchID == fid && doFire) {
                int remSize = (remFiles == null) ? 0 : remFiles.size();
                int addSize = (addFiles == null) ? 0 : addFiles.size();
                synchronized(fileCache) { <<<<<<<<<<<<<<<< ***remove*** from here and moved it 5 lines above
... ... ... ... ... ... ... ...
        }
    }
}

The downside is that the fileCache synchronized region is larger than before.