357278: deadlock when opening secure storage
Change-Id: I4f4048969d340115997c4ad2f150ff09e5cfba01
Task-Url: https://bugs.eclipse.org/bugs/show_bug.cgi?id=357278
diff --git a/org.eclipse.mylyn.commons.repositories.core/src/org/eclipse/mylyn/internal/commons/repositories/core/SecureCredentialsStore.java b/org.eclipse.mylyn.commons.repositories.core/src/org/eclipse/mylyn/internal/commons/repositories/core/SecureCredentialsStore.java
index 8be852d..956b0ee 100644
--- a/org.eclipse.mylyn.commons.repositories.core/src/org/eclipse/mylyn/internal/commons/repositories/core/SecureCredentialsStore.java
+++ b/org.eclipse.mylyn.commons.repositories.core/src/org/eclipse/mylyn/internal/commons/repositories/core/SecureCredentialsStore.java
@@ -176,11 +176,15 @@
}
protected ISecurePreferences getSecurePreferences() {
- ISecurePreferences securePreferences = SecurePreferencesFactory.getDefault().node(ID_NODE);
+ ISecurePreferences securePreferences = openSecurePreferences().node(ID_NODE);
securePreferences = securePreferences.node(EncodingUtils.encodeSlashes(getId()));
return securePreferences;
}
+ protected ISecurePreferences openSecurePreferences() {
+ return SecurePreferencesFactory.getDefault();
+ }
+
private void handle(StorageException e) {
if (!loggedStorageException) {
loggedStorageException = true;
diff --git a/org.eclipse.mylyn.commons.repositories.tests/src/org/eclipse/mylyn/commons/repositories/tests/core/SecureCredentialsStoreTest.java b/org.eclipse.mylyn.commons.repositories.tests/src/org/eclipse/mylyn/commons/repositories/tests/core/SecureCredentialsStoreTest.java
index aae921b..265cf13 100644
--- a/org.eclipse.mylyn.commons.repositories.tests/src/org/eclipse/mylyn/commons/repositories/tests/core/SecureCredentialsStoreTest.java
+++ b/org.eclipse.mylyn.commons.repositories.tests/src/org/eclipse/mylyn/commons/repositories/tests/core/SecureCredentialsStoreTest.java
@@ -12,14 +12,23 @@
package org.eclipse.mylyn.commons.repositories.tests.core;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
import java.util.Arrays;
+import org.eclipse.core.runtime.AssertionFailedException;
import org.eclipse.equinox.security.storage.ISecurePreferences;
import org.eclipse.equinox.security.storage.StorageException;
import org.eclipse.mylyn.commons.repositories.tests.support.DelegatingSecurePreferences;
import org.eclipse.mylyn.internal.commons.repositories.core.InMemoryCredentialsStore;
import org.eclipse.mylyn.internal.commons.repositories.core.SecureCredentialsStore;
+import org.eclipse.mylyn.internal.commons.repositories.ui.UiLocationService;
+import org.eclipse.mylyn.internal.commons.repositories.ui.UiSecureCredentialsStore;
+import org.eclipse.swt.widgets.Display;
+import org.eclipse.ui.PlatformUI;
import org.junit.Test;
/**
@@ -27,10 +36,14 @@
*/
public class SecureCredentialsStoreTest extends AbstractCredentialsStoreTest {
- private class StubSecureCredentialsStore extends SecureCredentialsStore {
+ private class StubSecureCredentialsStore extends UiSecureCredentialsStore {
DelegatingSecurePreferences delegate;
+ private boolean openSecurePreferencesCalled;
+
+ private Display display;
+
public StubSecureCredentialsStore() {
super(SecureCredentialsStore.class.getName());
}
@@ -59,6 +72,20 @@
return super.getInMemoryStore();
}
+ @Override
+ protected ISecurePreferences openSecurePreferences() {
+ openSecurePreferencesCalled = true;
+ display = Display.getCurrent();
+ return super.openSecurePreferences();
+ }
+
+ protected boolean wasOpenSecurePreferencesCalled() {
+ return openSecurePreferencesCalled;
+ }
+
+ protected Display getDisplay() {
+ return display;
+ }
}
@Test
@@ -138,4 +165,71 @@
assertEquals("value", store.get("key", null));
}
+ @Test
+ public void testopenSecurePreferencesThrowsExceptionOnUiThread() throws Exception {
+ runOnUiThread(new Runnable() {
+ public void run() {
+ assertNotNull(Display.getCurrent());
+ StubSecureCredentialsStore store = new StubSecureCredentialsStore();
+ try {
+ store.openSecurePreferences();
+ } catch (AssertionFailedException e) {// expected
+ return;
+ }
+ assertTrue(false);
+ }
+ });
+ }
+
+ @Test
+ public void testAccessSecureStoreOnUiThread() throws Exception {
+ runOnUiThread(new Runnable() {
+ public void run() {
+ assertNotNull(Display.getCurrent());
+ StubSecureCredentialsStore store = new StubSecureCredentialsStore();
+ assertFalse(store.wasOpenSecurePreferencesCalled());
+ store.put("key", "value", false);
+ assertEquals("value", store.get("key", null));
+ // check that openSecurePreferences was called but not from the UI thread
+ assertTrue(store.wasOpenSecurePreferencesCalled());
+ assertNull(store.getDisplay());
+ }
+ });
+ }
+
+ protected void runOnUiThread(final Runnable runnable) throws AssertionError {
+ final AssertionError assertionError[] = new AssertionError[1];
+ PlatformUI.getWorkbench().getDisplay().syncExec(new Runnable() {
+ public void run() {
+ try {
+ runnable.run();
+ } catch (AssertionError e) {
+ assertionError[0] = e;
+ }
+
+ }
+ });
+ if (assertionError[0] != null) {
+ throw assertionError[0];
+ }
+ }
+
+ @Test
+ public void testRunOnUiThread() throws Exception {
+ try {
+ runOnUiThread(new Runnable() {
+ public void run() {
+ assertTrue(false);
+ }
+ });
+ } catch (AssertionError e) {// expected
+ return;
+ }
+ assertTrue(false);
+ }
+
+ @Test
+ public void testUiLocationService() throws Exception {
+ assertTrue(new UiLocationService().getCredentialsStore("test") instanceof UiSecureCredentialsStore);
+ }
}
diff --git a/org.eclipse.mylyn.commons.repositories.ui/META-INF/MANIFEST.MF b/org.eclipse.mylyn.commons.repositories.ui/META-INF/MANIFEST.MF
index 95e82e4..95672ff 100644
--- a/org.eclipse.mylyn.commons.repositories.ui/META-INF/MANIFEST.MF
+++ b/org.eclipse.mylyn.commons.repositories.ui/META-INF/MANIFEST.MF
@@ -17,7 +17,8 @@
org.eclipse.mylyn.commons.core;bundle-version="3.8.0",
org.eclipse.mylyn.commons.repositories.core;bundle-version="1.0.0",
org.eclipse.mylyn.commons.ui;bundle-version="3.8.0",
- org.eclipse.mylyn.commons.workbench;bundle-version="3.8.0"
+ org.eclipse.mylyn.commons.workbench;bundle-version="3.8.0",
+ org.eclipse.equinox.security
Bundle-RequiredExecutionEnvironment: JavaSE-1.6
Bundle-ActivationPolicy: lazy
Export-Package: org.eclipse.mylyn.commons.repositories.ui;x-internal:=true,
diff --git a/org.eclipse.mylyn.commons.repositories.ui/src/org/eclipse/mylyn/internal/commons/repositories/ui/UiLocationService.java b/org.eclipse.mylyn.commons.repositories.ui/src/org/eclipse/mylyn/internal/commons/repositories/ui/UiLocationService.java
index f253bcf..ed2bfa1 100644
--- a/org.eclipse.mylyn.commons.repositories.ui/src/org/eclipse/mylyn/internal/commons/repositories/ui/UiLocationService.java
+++ b/org.eclipse.mylyn.commons.repositories.ui/src/org/eclipse/mylyn/internal/commons/repositories/ui/UiLocationService.java
@@ -25,7 +25,6 @@
import org.eclipse.mylyn.commons.repositories.core.auth.AuthenticationRequest;
import org.eclipse.mylyn.commons.repositories.core.auth.AuthenticationType;
import org.eclipse.mylyn.commons.repositories.core.auth.ICredentialsStore;
-import org.eclipse.mylyn.internal.commons.repositories.core.SecureCredentialsStore;
import org.eclipse.mylyn.internal.commons.repositories.ui.auth.RepositoryAuthenticator;
/**
@@ -38,7 +37,7 @@
public ICredentialsStore getCredentialsStore(String id) {
Assert.isNotNull(id);
- return new SecureCredentialsStore(id);
+ return new UiSecureCredentialsStore(id);
}
public Proxy getProxyForHost(String host, String proxyType) {
diff --git a/org.eclipse.mylyn.commons.repositories.ui/src/org/eclipse/mylyn/internal/commons/repositories/ui/UiSecureCredentialsStore.java b/org.eclipse.mylyn.commons.repositories.ui/src/org/eclipse/mylyn/internal/commons/repositories/ui/UiSecureCredentialsStore.java
new file mode 100644
index 0000000..8f002a6
--- /dev/null
+++ b/org.eclipse.mylyn.commons.repositories.ui/src/org/eclipse/mylyn/internal/commons/repositories/ui/UiSecureCredentialsStore.java
@@ -0,0 +1,71 @@
+/*******************************************************************************
+ * Copyright (c) 2013 Tasktop Technologies and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ * Tasktop Technologies - initial API and implementation
+ *******************************************************************************/
+
+package org.eclipse.mylyn.internal.commons.repositories.ui;
+
+import java.lang.reflect.InvocationTargetException;
+
+import org.eclipse.core.runtime.Assert;
+import org.eclipse.core.runtime.IProgressMonitor;
+import org.eclipse.core.runtime.NullProgressMonitor;
+import org.eclipse.equinox.security.storage.ISecurePreferences;
+import org.eclipse.jface.operation.IRunnableWithProgress;
+import org.eclipse.jface.operation.ModalContext;
+import org.eclipse.mylyn.internal.commons.repositories.core.SecureCredentialsStore;
+import org.eclipse.swt.widgets.Display;
+
+/**
+ * When this class is accessed on the UI thread, access to the secure store will occur on a background thread and the
+ * event loop will continue to run. This prevents a deadlock that can occur when accessing the secure store on the UI
+ * thread.
+ * <p>
+ * Clients should be extremely careful when accessing this class on the UI thread while holding onto a lock (such as
+ * from within synchronized methods). It is possible that the event loop will process an event that spawns another job
+ * (such as when the user opens a dialog which runs a background operation). If that job also tries to acquire the lock,
+ * there will be a deadlock. For this reason, clients should never hold a lock while accessing this class on the UI
+ * thread, unless they can be certain that nothing can cause the UI thread to wait for another job that attempts to
+ * acquire the lock.
+ *
+ * @author Sam Davis
+ */
+public class UiSecureCredentialsStore extends SecureCredentialsStore {
+
+ public UiSecureCredentialsStore(String id) {
+ super(id);
+ }
+
+ @Override
+ protected ISecurePreferences getSecurePreferences() {
+ if (Display.getCurrent() != null) {
+ // ensure we don't open the secure preferences on the UI thread as this can cause deadlock
+ final ISecurePreferences securePreferences[] = new ISecurePreferences[1];
+ try {
+ ModalContext.run(new IRunnableWithProgress() {
+ public void run(IProgressMonitor monitor) throws InvocationTargetException, InterruptedException {
+ securePreferences[0] = UiSecureCredentialsStore.super.getSecurePreferences();
+ }
+ }, true, new NullProgressMonitor(), Display.getCurrent());
+ } catch (InvocationTargetException e) {
+ throw new RuntimeException(e);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ return securePreferences[0];
+ }
+ return super.getSecurePreferences();
+ }
+
+ @Override
+ protected ISecurePreferences openSecurePreferences() {
+ Assert.isTrue(Display.getCurrent() == null);
+ return super.openSecurePreferences();
+ }
+}
