[115714] NullPointerException in Hashtable caused by FileBufferModelManager releaseModel

3 files changed, 107 insertions, 108 deletions

diff --git a/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/AbstractStructuredModel.java b/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/AbstractStructuredModel.java
index eae0077be7..952e4b39e2 100644
--- a/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/AbstractStructuredModel.java
+++ b/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/AbstractStructuredModel.java
@@ -222,34 +222,6 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 	}
 
 
-	private void _commonRelease() {
-
-		// TODO_future: I suspect ALL this type of logic should be
-		// on manager side.
-
-		if (factoryRegistry != null) {
-			factoryRegistry.release();
-		}
-		// if document as not been changed, we'll still be listening for
-		// first change. This is not a critical clean up, since presumanly
-		// whole model and document are "going away", but can make
-		// other memory leaks harder to find if we stay attached.
-		// (Note: my first thought was to set fStructuredDocument to null
-		// also,
-		// but there's others in shutdown process that still need to
-		// get it, in order to disconnect from it.)
-		if (fStructuredDocument != null) {
-			fStructuredDocument.removeDocumentChangedListener(fDirtyStateWatcher);
-			fStructuredDocument.removeDocumentAboutToChangeListener(fDocumentToModelNotifier);
-			fStructuredDocument.removeDocumentChangedListener(fDocumentToModelNotifier);
-		}
-
-		// we set document to null, mostly so
-		// any initiatilation done there can be undone.
-		// (likely needs some restructured to not need?)
-		setStructuredDocument(null);
-	}
-
 	/**
 	 * This method is just for getting an instance of the model manager of the
 	 * right Impl type, to be used "internally" for making protected calls
@@ -268,12 +240,12 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 	 * This API allows clients to declare that they are about to make a
 	 * "large" change to the model. This change might be in terms of content
 	 * or it might be in terms of the model id or base location. Note that in
-	 * the case of embedded calls, notification to listners is sent only once.
-	 * Note that the client who is making these changes has the responsibility
-	 * to restore the models state once finished with the changes. See
-	 * getMemento and restoreState. The method isModelStateChanging can be
-	 * used by a client to determine if the model is already in a change
-	 * sequence.
+	 * the case of embedded calls, notification to listeners is sent only
+	 * once. Note that the client who is making these changes has the
+	 * responsibility to restore the models state once finished with the
+	 * changes. See getMemento and restoreState. The method
+	 * isModelStateChanging can be used by a client to determine if the model
+	 * is already in a change sequence.
 	 */
 	public void aboutToChangeModel() {
 
@@ -295,7 +267,6 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 	public void addModelLifecycleListener(IModelLifecycleListener listener) {
 
 		synchronized (fListenerLock) {
-
 			if (fLifecycleNotificationManager == null) {
 				fLifecycleNotificationManager = new LifecycleNotificationManager();
 			}
@@ -1051,10 +1022,14 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 			throw new IllegalStateException(MODEL_MANAGER_NULL); //$NON-NLS-1$
 		}
 		else {
-			// be sure to check the shared state before releasing. (Since
-			// isShared assumes a count
-			// of 1 means not shared ... and we want our '1' to be that
-			// one.)
+			/*
+			 * Be sure to check the shared state before releasing. (Since
+			 * isShared assumes a count of 1 means not shared ... and we want
+			 * our '1' to be that one.) The problem, of course, is that
+			 * between pre-cycle notification and post-release notification,
+			 * the model could once again have become shared, rendering the
+			 * release notification incorrect.
+			 */
 			boolean isShared = isShared();
 
 			if (!isShared) {
@@ -1062,10 +1037,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 			}
 
 			_getModelManager().releaseFromEdit(getId());
-			// if no one else is using us, free up
-			// our resources
 			if (!isShared) {
-				_commonRelease();
 				signalPostLifeCycleListenerRelease(this);
 			}
 		}
@@ -1082,10 +1054,14 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 			throw new IllegalStateException(MODEL_MANAGER_NULL); //$NON-NLS-1$
 		}
 		else {
-			// be sure to check the shared state before
-			// releasing. (Since isShared assumes a count
-			// of 1 means not shared ... and we want
-			// our '1' to be that one.)
+			/*
+			 * Be sure to check the shared state before releasing. (Since
+			 * isShared assumes a count of 1 means not shared ... and we want
+			 * our '1' to be that one.) The problem, of course, is that
+			 * between pre-cycle notification and post-release notification,
+			 * the model could once again have become shared, rendering the
+			 * release notification incorrect.
+			 */
 			boolean isShared = isShared();
 
 			if (!isShared) {
@@ -1093,11 +1069,8 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 			}
 
 			_getModelManager().releaseFromRead(getId());
-			// if no one else is using us, free up
-			// an resources
+
 			if (!isShared) {
-				// factoryRegistry.release();
-				_commonRelease();
 				signalPostLifeCycleListenerRelease(this);
 			}
 		}
@@ -1129,7 +1102,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 	public void removeModelLifecycleListener(IModelLifecycleListener listener) {
 
 		// if manager is null, then none have been added, so
-		// no need to remove it.
+		// no need to remove any
 		if (fLifecycleNotificationManager == null)
 			return;
 		synchronized (fListenerLock) {
@@ -1170,7 +1143,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 
 
 	/**
-	 * A method that modififies the model's synchonization stamp to match the
+	 * A method that modifies the model's synchronization stamp to match the
 	 * resource. Turns out there's several ways of doing it, so this ensures a
 	 * common algorithm.
 	 */
@@ -1200,7 +1173,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 	 * etc., since its really up to the client to determine what's "new" about
 	 * a moved model. Caution: 'this' and 'newModel' may be the same object.
 	 * This is the case for current working with FileModelProvider, but have
-	 * left the dual argument for future possiblities.
+	 * left the dual argument for future possibilities.
 	 */
 	public void resourceMoved(IStructuredModel newModel) {
 
@@ -1222,7 +1195,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 		}
 
 		finally {
-			// we put end notificatin in finally block, so even if
+			// we put end notification in finally block, so even if
 			// error occurs during save, listeners are still notified,
 			// since their code could depend on receiving, to clean up
 			// some state, or coordinate other resources.
@@ -1244,7 +1217,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 			_getModelManager().saveModel(stringId, encodingRule);
 		}
 		finally {
-			// we put end notificatin in finally block, so even if
+			// we put end notification in finally block, so even if
 			// error occurs during save, listeners are still notified,
 			// since their code could depend on receiving, to clean up
 			// some state, or coordinate other resources.
@@ -1267,7 +1240,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 		}
 
 		finally {
-			// we put end notificatin in finally block, so even if
+			// we put end notification in finally block, so even if
 			// error occurs during save, listeners are still notified,
 			// since their code could depend on receiving, to clean up
 			// some state, or coordinate other resources.
@@ -1289,7 +1262,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 			_getModelManager().saveModel(iFile, stringId, encodingRule);
 		}
 		finally {
-			// we put end notificatin in finally block, so even if
+			// we put end notificatioon in finally block, so even if
 			// error occurs during save, listeners are still notified,
 			// since their code could depend on receiving, to clean up
 			// some state, or coordinate other resources.
@@ -1312,7 +1285,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 		}
 
 		finally {
-			// we put end notificatin in finally block, so even if
+			// we put end notification in finally block, so even if
 			// error occurs during save, listeners are still notified,
 			// since their code could depend on receiving, to clean up
 			// some state, or coordinate other resources.
@@ -1343,7 +1316,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 
 		// no need to process (set or fire event), if same value
 		if (fDirtyState != dirtyState) {
-			// prechange notificaiton
+			// pre-change notification
 			int type = ModelLifecycleEvent.MODEL_DIRTY_STATE | ModelLifecycleEvent.PRE_EVENT;
 			ModelLifecycleEvent modelLifecycleEvent = new ModelLifecycleEvent(this, type);
 			signalLifecycleEvent(modelLifecycleEvent);
@@ -1395,7 +1368,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 		// hashtable.
 		if (newId == null)
 			throw new IllegalArgumentException(SSECoreMessages.A_model_s_id_can_not_be_nu_EXC_); //$NON-NLS-1$ = "A model's id can not be null"
-		// To gaurd againt throwing a spurious ResourceInUse exception,
+		// To guard against throwing a spurious ResourceInUse exception,
 		// which can occur when two pieces of code both want to change the id,
 		// so the second request is spurious, we'll ignore any requests that
 		// attempt to change the id to what it already is ... note, we use
@@ -1410,7 +1383,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 		//
 		if (newId.equals(fId))
 			return;
-		// we must gaurd against reassigning an id to one that we already
+		// we must guard against reassigning an id to one that we already
 		// are managing.
 		if (getModelManager() != null) {
 			IStructuredModel newModel = getModelManager().getExistingModelForEdit(newId);
@@ -1438,7 +1411,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 		finally {
 			// make sure this finally is only executed if 'about to Change
 			// model' has
-			// ben executed.
+			// been executed.
 			changedModel();
 		}
 	}
@@ -1494,8 +1467,8 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 	 * Holds any data that the reinit procedure might find useful in
 	 * reinitializing the model. This is handy, since the reinitialization may
 	 * not take place at once, and some "old" data may be needed to properly
-	 * undo previous settings. Note: the parameter was intentially made to be
-	 * of type 'Object' so different models can use in different ways.
+	 * undo previous settings. Note: the parameter was intentionally made to
+	 * be of type 'Object' so different models can use in different ways.
 	 */
 	public void setReinitializeStateData(Object object) {
 
@@ -1515,7 +1488,7 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 			fStructuredDocument.removeDocumentChangedListener(fDirtyStateWatcher);
 			fStructuredDocument.removeDocumentAboutToChangeListener(fDocumentToModelNotifier);
 			fStructuredDocument.removeDocumentChangedListener(fDocumentToModelNotifier);
-			// prechange notificaiton
+			// prechange notification
 			lifeCycleNotification = true;
 			ModelLifecycleEvent modelLifecycleEvent = new DocumentChanged(ModelLifecycleEvent.PRE_EVENT, this, fStructuredDocument, newStructuredDocument);
 			signalLifecycleEvent(modelLifecycleEvent);
@@ -1565,11 +1538,10 @@ public abstract class AbstractStructuredModel implements IStructuredModel {
 	}
 
 	/**
-	 * to be called only be "friendly" classes, such as ModelManger, and
+	 * To be called only by "friendly" classes, such as ModelManager, and
 	 * subclasses.
 	 */
 	void signalLifecycleEvent(ModelLifecycleEvent event) {
-
 		if (fLifecycleNotificationManager == null)
 			return;
 		fLifecycleNotificationManager.signalLifecycleEvent(event);
diff --git a/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/ModelManagerImpl.java b/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/ModelManagerImpl.java
index ddd652713f..9349bc3f43 100644
--- a/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/ModelManagerImpl.java
+++ b/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/model/ModelManagerImpl.java
@@ -36,8 +36,11 @@ import org.eclipse.core.resources.IWorkspaceRoot;
 import org.eclipse.core.resources.ResourcesPlugin;
 import org.eclipse.core.runtime.CoreException;
 import org.eclipse.core.runtime.IPath;
+import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.core.runtime.Path;
+import org.eclipse.core.runtime.Platform;
+import org.eclipse.core.runtime.Status;
 import org.eclipse.jface.text.BadLocationException;
 import org.eclipse.jface.text.IDocument;
 import org.eclipse.jface.text.IRegion;
@@ -48,6 +51,7 @@ import org.eclipse.wst.sse.core.internal.FileBufferModelManager;
 import org.eclipse.wst.sse.core.internal.Logger;
 import org.eclipse.wst.sse.core.internal.NullMemento;
 import org.eclipse.wst.sse.core.internal.SSECoreMessages;
+import org.eclipse.wst.sse.core.internal.SSECorePlugin;
 import org.eclipse.wst.sse.core.internal.document.DocumentReader;
 import org.eclipse.wst.sse.core.internal.document.IDocumentLoader;
 import org.eclipse.wst.sse.core.internal.encoding.CodedIO;
@@ -1395,10 +1399,32 @@ public class ModelManagerImpl implements IModelManager {
 	private void discardModel(Object id, SharedObject sharedObject) {
 		fManagedObjects.remove(id);
 		IStructuredDocument structuredDocument = sharedObject.theSharedModel.getStructuredDocument();
+
+		if (structuredDocument == null) {
+			Platform.getLog(SSECorePlugin.getDefault().getBundle()).log(new Status(IStatus.ERROR, SSECorePlugin.ID, IStatus.ERROR, "Attempted to discard a structured model but the underlying document has already been set to null: " + sharedObject.theSharedModel.getBaseLocation(), null));
+		}
+
+		/*
+		 * This call (and setting the StructuredDocument to null) were
+		 * previously done within the model itself, but for concurrency it
+		 * must be done here during a synchronized release.
+		 */
+		sharedObject.theSharedModel.getFactoryRegistry().release();
+
+		/*
+		 * For structured documents originating from file buffers, disconnect
+		 * us from the file buffer, now.
+		 */
 		FileBufferModelManager.getInstance().releaseModel(structuredDocument);
-		// setting the document to null is required since some subclasses
-		// of model might have "cleanup" of listners, etc., to remove,
-		// which were initialized during the intial setStructuredDocument
+
+		/*
+		 * Setting the document to null is required since some subclasses of
+		 * model might have "cleanup" of listeners, etc., to remove, which
+		 * were initialized during the initial setStructuredDocument.
+		 * 
+		 * The model itself in particular may have internal listeners used to
+		 * coordinate the document with its own "structure".
+		 */
 		sharedObject.theSharedModel.setStructuredDocument(null);
 	}
 
@@ -1411,7 +1437,7 @@ public class ModelManagerImpl implements IModelManager {
 		SharedObject sharedObject = null;
 
 		if (id.equals(UNMANAGED_MODEL) || id.equals(DUPLICATED_MODEL)) {
-			// do nothing related to model managment.
+			// do nothing related to model management.
 		}
 		else {
 
diff --git a/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/provisional/IStructuredModel.java b/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/provisional/IStructuredModel.java
index cc789dbda9..ba79ac677f 100644
--- a/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/provisional/IStructuredModel.java
+++ b/bundles/org.eclipse.wst.sse.core/src/org/eclipse/wst/sse/core/internal/provisional/IStructuredModel.java
@@ -32,13 +32,19 @@ import org.eclipse.wst.sse.core.internal.util.URIResolver;
 
 
 /**
- * IStructuredModel's are mainly interesting by their extensions and
- * implementers. The main purposed of this abstraction it to provide a common
- * way to manage models that have an associated structured documnet.
+ * IStructuredModels are mainly interesting by their extensions and
+ * implementers. The main purposed of this abstraction is to provide a common
+ * means to manage models that have an associated structured document.
  * 
- * @plannedfor 1.0
+ * @plannedfor 2.0
  * 
+ * <p>
  * ISSUE: this interface needs ton of cleanup!
+ * </p>
+ * 
+ * <p>
+ * This interface is not intended to be implemented by clients.
+ * </p>
  */
 public interface IStructuredModel extends IAdaptable {
 
@@ -101,12 +107,12 @@ public interface IStructuredModel extends IAdaptable {
 
 	/**
 	 * This API allows a client controlled way of notifying all ModelEvent
-	 * listners that the model has been changed. This method is a matched pair
-	 * to aboutToChangeModel, and must be called after aboutToChangeModel ...
-	 * or some listeners could be left waiting indefinitely for the changed
-	 * event. So, its suggested that changedModel always be in a finally
-	 * clause. Likewise, a client should never call changedModel without
-	 * calling aboutToChangeModel first.
+	 * listeners that the model has been changed. This method is a matched
+	 * pair to aboutToChangeModel, and must be called after aboutToChangeModel
+	 * ... or some listeners could be left waiting indefinitely for the
+	 * changed event. So, its suggested that changedModel always be in a
+	 * finally clause. Likewise, a client should never call changedModel
+	 * without calling aboutToChangeModel first.
 	 * 
 	 * In the case of embedded calls, the notification is just sent once.
 	 * 
@@ -165,36 +171,29 @@ public interface IStructuredModel extends IAdaptable {
 	String getId();
 
 	/**
-	 * 
+	 * @param offset
+	 *            a text offset within the structured document
+	 * @return an IndexedRegion containing this offset or null if one could
+	 *         not be found
 	 */
 	IndexedRegion getIndexedRegion(int offset);
 
-	/**
-	 * ContentTypeDescription provides an object that describes what the
-	 * content of the file is, e.g. HTML, XML, etc. Compare with
-	 * getExternalFileTypeDescription. Though they both return objects of type
-	 * ContentTypeDescription, the external file type is intended to denote
-	 * JSP, regardless of what the content of that JSP file is. Even for a JSP
-	 * file, the ContentTypeDescription will be set according to that file's
-	 * "internal" contents.
-	 * 
-	 * @return ContentTypeDescription
-	 */
 	IModelHandler getModelHandler();
 
 	IModelManager getModelManager();
 
 	/**
-	 * This function returns the reference count of underlying model.
-	 * 
 	 * @param id
 	 *            Object The id of the model TODO: try to refine the design
 	 *            not to use this function
+	 * 
+	 * @return the reference count of underlying model
 	 */
 	int getReferenceCount();
 
 	/**
-	 * This function returns the reference count of underlying model.
+	 * This function returns the edit-responsible reference count of
+	 * underlying model.
 	 * 
 	 * @param id
 	 *            Object The id of the model TODO: try to refine the design
@@ -203,7 +202,7 @@ public interface IStructuredModel extends IAdaptable {
 	int getReferenceCountForEdit();
 
 	/**
-	 * This function returns the reference count of underlying model.
+	 * This function returns the reader reference count of underlying model.
 	 * 
 	 * @param id
 	 *            Object The id of the model TODO: try to refine the design
@@ -215,6 +214,8 @@ public interface IStructuredModel extends IAdaptable {
 
 	/**
 	 * Get URI resolution helper
+	 * 
+	 * @deprecated
 	 */
 	URIResolver getResolver();
 
@@ -279,7 +280,7 @@ public interface IStructuredModel extends IAdaptable {
 	 * newInstance is similar to clone, except that the newInstance contains
 	 * no content. Its purpose is so clients can get a temporary, unmanaged,
 	 * model of the same "type" as the original. Note: the client may still
-	 * need to do some intialization of the model returned by newInstance,
+	 * need to do some initialization of the model returned by newInstance,
 	 * depending on desired use. For example, the only factories in the
 	 * newInstance are those that would be normally be created for a model of
 	 * the given contentType. Others are not copied automatically, and if
@@ -288,11 +289,11 @@ public interface IStructuredModel extends IAdaptable {
 	IStructuredModel newInstance() throws IOException;
 
 	/**
-	 * Performs a reinit procedure. For this model. Note for future: there may
-	 * be a day where the model returned from this method is a different
-	 * instance than the instance it was called on. This will occur when there
-	 * is full support for "save as" type functions, where the model could
-	 * theoretically change completely.
+	 * Performs a reinitialization procedure. For this model. Note for future:
+	 * there may be a day where the model returned from this method is a
+	 * different instance than the instance it was called on. This will occur
+	 * when there is full support for "save as" type functions, where the
+	 * model could theoretically change completely.
 	 */
 	IStructuredModel reinit() throws IOException;
 
@@ -313,7 +314,7 @@ public interface IStructuredModel extends IAdaptable {
 	/**
 	 * This function replenishes the model with the resource without saving
 	 * any possible changes. It is used when one editor may be closing, and
-	 * specifially says not to save the model, but another "display" of the
+	 * specifically says not to save the model, but another "display" of the
 	 * model still needs to hang on to some model, so needs a fresh copy.
 	 * 
 	 * Only valid for use with managed models.
@@ -325,7 +326,7 @@ public interface IStructuredModel extends IAdaptable {
 	void removeModelStateListener(IModelStateListener listener);
 
 	/**
-	 * A method that modififies the model's synchonization stamp to match the
+	 * A method that modifies the model's synchronization stamp to match the
 	 * resource. Turns out there's several ways of doing it, so this ensures a
 	 * common algorithm.
 	 */
@@ -368,11 +369,11 @@ public interface IStructuredModel extends IAdaptable {
 	void setReinitializeNeeded(boolean b);
 
 	/**
-	 * Holds any data that the reinit procedure might find useful in
+	 * Holds any data that the reinitialization procedure might find useful in
 	 * reinitializing the model. This is handy, since the reinitialization may
 	 * not take place at once, and some "old" data may be needed to properly
-	 * undo previous settings. Note: the parameter was intentially made to be
-	 * of type 'Object' so different models can use in different ways.
+	 * undo previous settings. Note: the parameter was intentionally made to
+	 * be of type 'Object' so different models can use in different ways.
 	 */
 	void setReinitializeStateData(Object object);