Bug 342711 - PreferenceConverter is not thread-safe

Running Display-thread-only code in a static initializer block can cause
the workbench to crash on startup.

Change-Id: I08722533895f0c1c943b2f0622c30ac7024ea06c
Signed-off-by: Stefan Xenos <sxenos@gmail.com>

4 files changed, 72 insertions, 35 deletions

diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/preference/PreferenceConverter.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/preference/PreferenceConverter.java
index d6f46822b9a..d85abe5e65b 100644
--- a/bundles/org.eclipse.jface/src/org/eclipse/jface/preference/PreferenceConverter.java
+++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/preference/PreferenceConverter.java
@@ -66,30 +66,43 @@ public class PreferenceConverter {
 
     private static final String ENTRY_SEPARATOR = ";"; //$NON-NLS-1$
 
-    /**
-     * The default-default value for <code>FontData[]</code> preferences.
-     */
-    public static final FontData[] FONTDATA_ARRAY_DEFAULT_DEFAULT;
-
-    /**
-     * The default-default value for <code>FontData</code> preferences.
-     */
-    public static final FontData FONTDATA_DEFAULT_DEFAULT;
-    static {
-		Display display = Display.getCurrent();
-		if (display == null) {
-			display = Display.getDefault ();
-		}
-
-        FONTDATA_ARRAY_DEFAULT_DEFAULT = display.getSystemFont().getFontData();
-        /**
-         * The default-default value for <code>FontData</code> preferences.
-         * This is left in for compatibility purposes. It is recommended that
-         * FONTDATA_ARRAY_DEFAULT_DEFAULT is actually used.
-         */
+	/**
+	 * The default-default value for <code>FontData[]</code> preferences.
+	 * Read-only.
+	 *
+	 * @deprecated this is not thread-safe and may contain invalid data at
+	 *             startup. Call {@link #getFontDataArrayDefaultDefault()} from
+	 *             the UI thread instead.
+	 */
+	@Deprecated
+	public static FontData[] FONTDATA_ARRAY_DEFAULT_DEFAULT;
 
-        FONTDATA_DEFAULT_DEFAULT = FONTDATA_ARRAY_DEFAULT_DEFAULT[0];
-    }
+	/**
+	 * The default-default value for <code>FontData</code> preferences.
+	 * Read-only.
+	 *
+	 * @deprecated this is not thread-safe and may contain invalid data at
+	 *             startup. Call {@link #getFontDataArrayDefaultDefault()}} from
+	 *             the UI thread instead.
+	 */
+	@Deprecated
+	public static FontData FONTDATA_DEFAULT_DEFAULT;
+
+	private static FontData[] fontDataArrayDefaultDefault;
+
+	static {
+		Display display = Display.getDefault();
+		display.asyncExec(new Runnable() {
+			@Override
+			public void run() {
+				// Ensure that the deprecated FONTDATA_DEFAULT_DEFAULT and
+				// FONTDATA_ARRAY_DEFAULT values
+				// are initialized as soon as possible
+				FONTDATA_ARRAY_DEFAULT_DEFAULT = getFontDataArrayDefaultDefault();
+				FONTDATA_DEFAULT_DEFAULT = getFontDataArrayDefaultDefault()[0];
+			}
+		});
+	}
 
 	/**
 	 * private constructor to prevent instantiation.
@@ -126,8 +139,10 @@ public class PreferenceConverter {
      * @since 3.0
      */
     public static FontData[] basicGetFontData(String value) {
+		FontData[] defaultResult = getFontDataArrayDefaultDefault();
+
         if (IPreferenceStore.STRING_DEFAULT_DEFAULT.equals(value)) {
-			return FONTDATA_ARRAY_DEFAULT_DEFAULT;
+			return defaultResult;
 		}
 
         //Read in all of them to get the value
@@ -139,9 +154,9 @@ public class PreferenceConverter {
             try {
                 fontData[i] = new FontData(tokenizer.nextToken());
             } catch (SWTException error) {
-                return FONTDATA_ARRAY_DEFAULT_DEFAULT;
+				return defaultResult;
             } catch (IllegalArgumentException error) {
-                return FONTDATA_ARRAY_DEFAULT_DEFAULT;
+				return defaultResult;
             }
         }
         return fontData;
@@ -302,6 +317,24 @@ public class PreferenceConverter {
         return basicGetFontData(store.getString(name));
     }
 
+	/**
+	 * The default-default value for <code>FontData[]</code> preferences. Must
+	 * be called from the UI thread.
+	 *
+	 * @return the default-default value for <code>FontData[]</code>
+	 *         preferences.
+	 * @since 3.12
+	 */
+	public static FontData[] getFontDataArrayDefaultDefault() {
+		Display display = Display.getCurrent();
+
+		if (fontDataArrayDefaultDefault == null) {
+			fontDataArrayDefaultDefault = display.getSystemFont().getFontData();
+		}
+
+		return fontDataArrayDefaultDefault;
+	}
+
     /**
      * Returns the current value of the first entry of the
      * font-valued preference with the
diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/ThemeElementHelper.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/ThemeElementHelper.java
index 8449c40e1be..3c0f4077ccd 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/ThemeElementHelper.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/ThemeElementHelper.java
@@ -133,7 +133,7 @@ public final class ThemeElementHelper {
         }
 
         if (setInRegistry) {
-			if (prefFont == null || prefFont == PreferenceConverter.FONTDATA_ARRAY_DEFAULT_DEFAULT) {
+			if (prefFont == null || prefFont == PreferenceConverter.getFontDataArrayDefaultDefault()) {
 				if (definition.getValue() != null) {
 					prefFont = definition.getValue();
 				} else if (definition.getDefaultsTo() != null) {
diff --git a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/WorkbenchThemeManager.java b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/WorkbenchThemeManager.java
index 66bc6306bc4..b0e649c2043 100644
--- a/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/WorkbenchThemeManager.java
+++ b/bundles/org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal/themes/WorkbenchThemeManager.java
@@ -453,7 +453,7 @@ public class WorkbenchThemeManager extends EventManager implements
 				if (def.isOverridden()) {
 					def.resetToDefaultValue();
 					fontRegistry.put(def.getId(), def.getValue() != null ? def.getValue()
-							: PreferenceConverter.FONTDATA_ARRAY_DEFAULT_DEFAULT);
+							: PreferenceConverter.getFontDataArrayDefaultDefault());
 				}
 			}
 			for (ColorDefinition def : themeRegistry.getColors()) {
diff --git a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/themes/WorkbenchThemeChangedHandlerTest.java b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/themes/WorkbenchThemeChangedHandlerTest.java
index d64001aa540..dc4420ae2ab 100644
--- a/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/themes/WorkbenchThemeChangedHandlerTest.java
+++ b/tests/org.eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/themes/WorkbenchThemeChangedHandlerTest.java
@@ -11,7 +11,6 @@
 
 package org.eclipse.ui.tests.themes;
 
-import static org.eclipse.jface.preference.PreferenceConverter.FONTDATA_ARRAY_DEFAULT_DEFAULT;
 import static org.eclipse.ui.internal.themes.WorkbenchThemeManager.EMPTY_COLOR_VALUE;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyObject;
@@ -25,11 +24,10 @@ import static org.mockito.Mockito.verify;
 
 import java.util.Arrays;
 
-import junit.framework.TestCase;
-
 import org.eclipse.e4.ui.css.swt.theme.ITheme;
 import org.eclipse.e4.ui.services.IStylingEngine;
 import org.eclipse.jface.preference.IPreferenceStore;
+import org.eclipse.jface.preference.PreferenceConverter;
 import org.eclipse.jface.resource.ColorRegistry;
 import org.eclipse.jface.resource.FontRegistry;
 import org.eclipse.swt.graphics.FontData;
@@ -42,6 +40,8 @@ import org.eclipse.ui.internal.themes.WorkbenchThemeManager;
 import org.eclipse.ui.internal.themes.WorkbenchThemeManager.WorkbenchThemeChangedHandler;
 import org.osgi.service.event.Event;
 
+import junit.framework.TestCase;
+
 /**
  * @since 3.5
  *
@@ -92,7 +92,8 @@ public class WorkbenchThemeChangedHandlerTest extends TestCase {
 		verify(stylingEngine, times(1)).style(colorDefinition);
 
 		verify(fontRegistry, times(2)).put(eq("fontDefinition1"), any(FontData[].class));
-		verify(fontRegistry, times(1)).put(eq("fontDefinition1"), eq(FONTDATA_ARRAY_DEFAULT_DEFAULT));
+		verify(fontRegistry, times(1)).put(eq("fontDefinition1"),
+				eq(PreferenceConverter.getFontDataArrayDefaultDefault()));
 		verify(fontRegistry, never()).put(eq("fontDefinition2"), any(FontData[].class));
 		verify(colorRegistry, times(2)).put(eq("colorDefinition"), any(RGB.class));
 		verify(colorRegistry, times(1)).put(eq("colorDefinition"), eq(EMPTY_COLOR_VALUE));
@@ -282,9 +283,11 @@ public class WorkbenchThemeChangedHandlerTest extends TestCase {
 		verify(stylingEngine, times(1)).style(colorDefinition2);
 
 		verify(fontRegistry, times(2)).put(eq("fontDefinition1"), any(FontData[].class));
-		verify(fontRegistry, times(1)).put(eq("fontDefinition1"), eq(FONTDATA_ARRAY_DEFAULT_DEFAULT));
+		verify(fontRegistry, times(1)).put(eq("fontDefinition1"),
+				eq(PreferenceConverter.getFontDataArrayDefaultDefault()));
 		verify(fontRegistry, times(2)).put(eq("fontDefinition2"), any(FontData[].class));
-		verify(fontRegistry, times(1)).put(eq("fontDefinition2"), eq(FONTDATA_ARRAY_DEFAULT_DEFAULT));
+		verify(fontRegistry, times(1)).put(eq("fontDefinition2"),
+				eq(PreferenceConverter.getFontDataArrayDefaultDefault()));
 		verify(colorRegistry, times(2)).put(eq("colorDefinition1"), any(RGB.class));
 		verify(colorRegistry, times(1)).put(eq("colorDefinition1"), eq(EMPTY_COLOR_VALUE));
 		verify(colorRegistry, times(2)).put(eq("colorDefinition2"), any(RGB.class));
@@ -354,7 +357,8 @@ public class WorkbenchThemeChangedHandlerTest extends TestCase {
 		//then
 		verify(fontDefinition1, times(1)).isOverridden();
 		verify(fontDefinition1, times(1)).resetToDefaultValue();
-		verify(fontRegistry, times(1)).put(fontDefinition1.getId(), FONTDATA_ARRAY_DEFAULT_DEFAULT);
+		verify(fontRegistry, times(1)).put(fontDefinition1.getId(),
+				PreferenceConverter.getFontDataArrayDefaultDefault());
 
 		verify(fontDefinition2, times(1)).isOverridden();
 		verify(fontDefinition2, times(1)).resetToDefaultValue();