Review comments.
draft
--- a/mobile/android/base/java/org/mozilla/gecko/push/PushClient.java
+++ b/mobile/android/base/java/org/mozilla/gecko/push/PushClient.java
@@ -32,43 +32,53 @@ public class PushClient {
}
private final AutopushClient autopushClient;
public PushClient(String serverURI) {
this.autopushClient = new AutopushClient(serverURI, Utils.newSynchronousExecutor());
}
+ /**
+ * Each instance is <b>single-use</b>! Exactly one delegate method should be invoked once,
+ * but we take care to handle multiple invocations (favoring the earliest), just to be safe.
+ */
protected static class Delegate<T> implements AutopushClient.RequestDelegate<T> {
Object result; // Oh, for an algebraic data type when you need one!
@SuppressWarnings("unchecked")
public T responseOrThrow() throws LocalException, AutopushClientException {
if (result instanceof LocalException) {
throw (LocalException) result;
}
if (result instanceof AutopushClientException) {
throw (AutopushClientException) result;
}
return (T) result;
}
@Override
public void handleError(Exception e) {
- result = new LocalException(e);
+ if (result == null) {
+ result = new LocalException(e);
+ }
}
@Override
public void handleFailure(AutopushClientException e) {
- result = e;
+ if (result == null) {
+ result = e;
+ }
}
@Override
public void handleSuccess(T response) {
- result = response;
+ if (result == null) {
+ result = response;
+ }
}
}
public RegisterUserAgentResponse registerUserAgent(@NonNull String token) throws LocalException, AutopushClientException {
final Delegate<RegisterUserAgentResponse> delegate = new Delegate<>();
autopushClient.registerUserAgent(token, delegate);
return delegate.responseOrThrow();
}
--- a/mobile/android/base/java/org/mozilla/gecko/push/PushManager.java
+++ b/mobile/android/base/java/org/mozilla/gecko/push/PushManager.java
@@ -4,28 +4,34 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package org.mozilla.gecko.push;
import android.support.annotation.NonNull;
import android.util.Log;
import org.json.JSONObject;
+import org.mozilla.gecko.AppConstants;
import org.mozilla.gecko.gcm.GcmTokenClient;
import org.mozilla.gecko.push.autopush.AutopushClientException;
import org.mozilla.gecko.util.ThreadUtils;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
/**
- * XXX fill comment.
+ * The push manager advances push registrations, ensuring that the upstream autopush endpoint has
+ * a fresh GCM token. It brokers channel subscription requests to the upstream and maintains
+ * local state.
+ * <p/>
+ * This class is not thread safe. An individual instance should be accessed on a single
+ * (background) thread.
*/
public class PushManager {
public static final long TIME_BETWEEN_AUTOPUSH_UAID_REGISTRATION_IN_MILLIS = 7 * 24 * 60 * 60 * 1000L; // One week.
public static class ProfileNeedsConfigurationException extends Exception {
private static final long serialVersionUID = 3326738888L;
public ProfileNeedsConfigurationException() {
@@ -60,17 +66,17 @@ public class PushManager {
}
}
return null;
}
public Map<String, PushSubscription> allSubscriptionsForProfile(String profileName) {
final PushRegistration registration = state.getRegistration(profileName);
if (registration == null) {
- return new HashMap<>();
+ return Collections.EMPTY_MAP;
}
return Collections.unmodifiableMap(registration.subscriptions);
}
public PushRegistration registerUserAgent(final @NonNull String profileName, final long now) throws ProfileNeedsConfigurationException, AutopushClientException, PushClient.LocalException, GcmTokenClient.NeedsGooglePlayServicesException, IOException {
Log.i(LOG_TAG, "Registering user agent for profile named: " + profileName);
return advanceRegistration(profileName, now);
}
@@ -109,17 +115,17 @@ public class PushManager {
throw new IllegalStateException("Cannot subscribeChannel with null uaid or secret!");
}
// Verify endpoint is not null?
final PushClient pushClient = pushClientFactory.getPushClient(registration.autopushEndpoint, registration.debug);
final SubscribeChannelResponse result = pushClient.subscribeChannel(uaid, secret);
if (registration.debug) {
- // Logger.pii(LOG_TAG, "Got chid: " + result.channelID + " and endpoint: " + result.endpoint);
+ Log.i(LOG_TAG, "Got chid: " + result.channelID + " and endpoint: " + result.endpoint);
} else {
Log.i(LOG_TAG, "Got chid and endpoint.");
}
final PushSubscription subscription = new PushSubscription(result.channelID, profileName, result.endpoint, service, serviceData);
registration.putSubscription(result.channelID, subscription);
state.checkpoint();
@@ -130,29 +136,28 @@ public class PushManager {
Log.i(LOG_TAG, "Unsubscribing from channel with chid: " + chid);
final PushRegistration registration = registrationForSubscription(chid);
if (registration == null) {
Log.w(LOG_TAG, "Cannot find registration corresponding to subscription; not unregistering remote subscription: " + chid);
return null;
}
+ // We remove the local subscription before the remote subscription: without the local
+ // subscription we'll ignoring incoming messages, and after some amount of time the
+ // server will expire the channel due to non-activity. This is also Desktop's approach.
final PushSubscription subscription = registration.removeSubscription(chid);
state.checkpoint();
if (subscription == null) {
// This should never happen.
- registration.removeSubscription(chid);
- state.checkpoint();
-
- Log.e(LOG_TAG, "Subscription does not exist: " + chid);
+ Log.e(LOG_TAG, "Subscription did not exist: " + chid);
return null;
}
-
final String uaid = registration.uaid.value;
final String secret = registration.secret;
if (uaid == null || secret == null) {
Log.e(LOG_TAG, "Cannot unsubscribeChannel with null registration uaid or secret!");
return null;
}
final PushClient pushClient = pushClientFactory.getPushClient(registration.autopushEndpoint, registration.debug);
@@ -174,19 +179,19 @@ public class PushManager {
public PushRegistration configure(final @NonNull String profileName, final @NonNull String endpoint, final boolean debug, final long now) {
Log.i(LOG_TAG, "Updating configuration.");
final PushRegistration registration = state.getRegistration(profileName);
final PushRegistration newRegistration;
if (registration != null) {
if (!endpoint.equals(registration.autopushEndpoint)) {
if (debug) {
- Log.i(LOG_TAG, "Push configuration autopushEndpoint changed!");
+ Log.i(LOG_TAG, "Push configuration autopushEndpoint changed! Was: " + registration.autopushEndpoint + "; now: " + endpoint);
} else {
- Log.i(LOG_TAG, "Push configuration autopushEndpoint changed! Was: " + registration.autopushEndpoint + "; now: " + endpoint);
+ Log.i(LOG_TAG, "Push configuration autopushEndpoint changed!");
}
newRegistration = new PushRegistration(endpoint, debug, Fetched.now(null), null);
if (registration.uaid.value != null) {
// New endpoint! All registrations and subscriptions have been dropped, and
// should be removed remotely.
unregisterUserAgentOnBackgroundThread(registration);
@@ -194,19 +199,19 @@ public class PushManager {
} else if (debug != registration.debug) {
Log.i(LOG_TAG, "Push configuration debug changed: " + debug);
newRegistration = registration.withDebug(debug);
} else {
newRegistration = registration;
}
} else {
if (debug) {
- Log.i(LOG_TAG, "Push configuration set!");
+ Log.i(LOG_TAG, "Push configuration set: " + endpoint + "; debug: " + debug);
} else {
- Log.i(LOG_TAG, "Push configuration set: " + endpoint + "; debug: " + debug);
+ Log.i(LOG_TAG, "Push configuration set!");
}
newRegistration = new PushRegistration(endpoint, debug, new Fetched(null, now), null);
}
if (newRegistration != registration) {
state.putRegistration(profileName, newRegistration);
state.checkpoint();
}
@@ -223,81 +228,79 @@ public class PushManager {
Log.i(LOG_TAG, "Unregistered user agent with uaid: " + registration.uaid.value);
} catch (PushClient.LocalException | AutopushClientException e) {
Log.w(LOG_TAG, "Failed to unregister user agent with uaid; ignoring: " + registration.uaid.value, e);
}
}
});
}
- public static final String MOZ_ANDROID_GCM_SENDER_ID = "829133274407";
-
protected @NonNull PushRegistration advanceRegistration(final @NonNull String profileName, final long now) throws ProfileNeedsConfigurationException, AutopushClientException, PushClient.LocalException, GcmTokenClient.NeedsGooglePlayServicesException, IOException {
final PushRegistration registration = state.getRegistration(profileName);
if (registration == null || registration.autopushEndpoint == null) {
Log.i(LOG_TAG, "Cannot advance to registered: registration needs configuration.");
throw new ProfileNeedsConfigurationException();
}
return advanceRegistration(registration, profileName, now);
}
protected @NonNull PushRegistration advanceRegistration(final PushRegistration registration, final @NonNull String profileName, final long now) throws AutopushClientException, PushClient.LocalException, GcmTokenClient.NeedsGooglePlayServicesException, IOException {
- final Fetched gcmToken = gcmClient.getToken(MOZ_ANDROID_GCM_SENDER_ID);
- Log.w(LOG_TAG, "Using GCM token: " + gcmToken.value); // XXX PII
+ final Fetched gcmToken = gcmClient.getToken(AppConstants.MOZ_ANDROID_GCM_SENDERID, registration.debug);
final PushClient pushClient = pushClientFactory.getPushClient(registration.autopushEndpoint, registration.debug);
if (registration.uaid.value == null) {
if (registration.debug) {
- // Logger.pii(LOG_TAG, "No uaid; requesting from autopush endpoint: " + registration.autopushEndpoint);
+ Log.i(LOG_TAG, "No uaid; requesting from autopush endpoint: " + registration.autopushEndpoint);
} else {
Log.i(LOG_TAG, "No uaid: requesting from autopush endpoint.");
}
final RegisterUserAgentResponse result = pushClient.registerUserAgent(gcmToken.value);
if (registration.debug) {
- // Logger.pii(LOG_TAG, "Got uaid: " + result.uaid + " and secret: " + result.secret);
+ Log.i(LOG_TAG, "Got uaid: " + result.uaid + " and secret: " + result.secret);
} else {
Log.i(LOG_TAG, "Got uaid and secret.");
}
final long nextNow = System.currentTimeMillis();
final PushRegistration nextRegistration = registration.withUserAgentID(result.uaid, result.secret, nextNow);
state.putRegistration(profileName, nextRegistration);
state.checkpoint();
return advanceRegistration(nextRegistration, profileName, nextNow);
- } else if (registration.uaid.timestamp + TIME_BETWEEN_AUTOPUSH_UAID_REGISTRATION_IN_MILLIS < now
+ }
+
+ if (registration.uaid.timestamp + TIME_BETWEEN_AUTOPUSH_UAID_REGISTRATION_IN_MILLIS < now
|| registration.uaid.timestamp < gcmToken.timestamp) {
if (registration.debug) {
- // Logger.pii(LOG_TAG, "Stale uaid; re-registering with autopush endpoint: " + registration.autopushEndpoint);
+ Log.i(LOG_TAG, "Stale uaid; re-registering with autopush endpoint: " + registration.autopushEndpoint);
} else {
Log.i(LOG_TAG, "Stale uaid: re-registering with autopush endpoint.");
}
pushClient.reregisterUserAgent(registration.uaid.value, registration.secret, gcmToken.value);
Log.i(LOG_TAG, "Re-registered uaid and secret.");
final long nextNow = System.currentTimeMillis();
final PushRegistration nextRegistration = registration.withUserAgentID(registration.uaid.value, registration.secret, nextNow);
state.putRegistration(profileName, nextRegistration);
state.checkpoint();
return advanceRegistration(nextRegistration, profileName, nextNow);
- } else {
- Log.d(LOG_TAG, "Existing uaid is fresh; no need to request from autopush endpoint.");
}
+ Log.d(LOG_TAG, "Existing uaid is fresh; no need to request from autopush endpoint.");
return registration;
}
public void invalidateGcmToken() {
gcmClient.invalidateToken();
}
public void startup(long now) {
try {
Log.i(LOG_TAG, "Startup: requesting GCM token.");
- gcmClient.getToken(MOZ_ANDROID_GCM_SENDER_ID); // For side-effects.
+ gcmClient.getToken(AppConstants.MOZ_ANDROID_GCM_SENDERID, false); // For side-effects.
} catch (GcmTokenClient.NeedsGooglePlayServicesException e) {
// Requires user intervention. At App startup, we don't want to address this. In
// response to user activity, we do want to try to have the user address this.
Log.w(LOG_TAG, "Startup: needs Google Play Services. Ignoring until GCM is requested in response to user activity.");
return;
} catch (IOException e) {
// We're temporarily unable to get a GCM token. There's nothing to be done; we'll
// try to advance the App's state in response to user activity or at next startup.
--- a/mobile/android/base/java/org/mozilla/gecko/util/GeckoBackgroundThread.java
+++ b/mobile/android/base/java/org/mozilla/gecko/util/GeckoBackgroundThread.java
@@ -4,17 +4,17 @@
package org.mozilla.gecko.util;
import android.os.Handler;
import android.os.Looper;
import java.util.concurrent.SynchronousQueue;
-public final class GeckoBackgroundThread extends Thread {
+final class GeckoBackgroundThread extends Thread {
private static final String LOOPER_NAME = "GeckoBackgroundThread";
// Guarded by 'GeckoBackgroundThread.class'.
private static Handler handler;
private static Thread thread;
// The initial Runnable to run on the new thread. Its purpose
// is to avoid us having to wait for the new thread to start.
@@ -47,18 +47,17 @@ public final class GeckoBackgroundThread
thread = new GeckoBackgroundThread(initialRunnable);
ThreadUtils.setBackgroundThread(thread);
thread.setDaemon(true);
thread.start();
}
// Get a Handler for a looper thread, or create one if it doesn't yet exist.
- // Public for testing only.
- public static synchronized Handler getHandler() {
+ /*package*/ static synchronized Handler getHandler() {
if (thread == null) {
startThread(null);
}
while (handler == null) {
try {
GeckoBackgroundThread.class.wait();
} catch (final InterruptedException e) {
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/push/TestPushManager.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/push/TestPushManager.java
@@ -3,49 +3,45 @@
package org.mozilla.gecko.push;
import org.json.JSONObject;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mozilla.gecko.AppConstants;
import org.mozilla.gecko.background.testhelpers.TestRunner;
import org.mozilla.gecko.gcm.GcmTokenClient;
-import org.mozilla.gecko.util.GeckoBackgroundThread;
import org.robolectric.RuntimeEnvironment;
-import org.robolectric.internal.ShadowExtractor;
-import org.robolectric.shadows.ShadowLooper;
import java.util.UUID;
import static org.mockito.Matchers.anyBoolean;
import static org.mockito.Matchers.anyString;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
-// TODO: XXX: notes about timestamps and clocks.
-// TODO: XXX: notes about threading and concurrency.
@RunWith(TestRunner.class)
public class TestPushManager {
private PushState state;
private GcmTokenClient gcmTokenClient;
private PushClient pushClient;
private PushManager manager;
@Before
public void setUp() throws Exception {
state = new PushState(RuntimeEnvironment.application, "test.json");
gcmTokenClient = mock(GcmTokenClient.class);
- doReturn("opaque-gcm-token").when(gcmTokenClient).getToken(anyString());
+ doReturn(new Fetched("opaque-gcm-token", System.currentTimeMillis())).when(gcmTokenClient).getToken(anyString(), anyBoolean());
// Configure a mock PushClient.
pushClient = mock(PushClient.class);
doReturn(new RegisterUserAgentResponse("opaque-uaid", "opaque-secret"))
.when(pushClient)
.registerUserAgent(anyString());
doReturn(new SubscribeChannelResponse("opaque-chid", "https://localhost:8085/opaque-push-endpoint"))
@@ -106,43 +102,37 @@ public class TestPushManager {
// Changing the debug flag should update but not try to unregister the User Agent.
registration = manager.configure("default", "http://localhost:8082", true, System.currentTimeMillis());
assertRegistered(registration, "http://localhost:8082", true);
// Changing the configuration endpoint should update and try to unregister the User Agent.
registration = manager.configure("default", "http://localhost:8083", true, System.currentTimeMillis());
assertOnlyConfigured(registration, "http://localhost:8083", true);
-
-// // But the configuration endpoint should unregister the correct User Agent.
-// registration = manager.configure("default", "http://localhost:8083", false, System.currentTimeMillis());
-// assertOnlyConfigured(registration, "http://localhost:8083", false);
-// // We shouldn't touch the other profile's registration.
-// assertRegistered(registration1, "http://localhost:8081", false);
}
@Test
public void testRegisterMultipleProfiles() throws Exception {
PushRegistration registration1 = manager.configure("default1", "http://localhost:8081", true, System.currentTimeMillis());
PushRegistration registration2 = manager.configure("default2", "http://localhost:8082", true, System.currentTimeMillis());
assertOnlyConfigured(registration1, "http://localhost:8081", true);
assertOnlyConfigured(registration2, "http://localhost:8082", true);
- verify(gcmTokenClient, times(0)).getToken(anyString());
+ verify(gcmTokenClient, times(0)).getToken(anyString(), anyBoolean());
registration1 = manager.registerUserAgent("default1", System.currentTimeMillis());
assertRegistered(registration1, "http://localhost:8081", true);
registration2 = manager.registerUserAgent("default2", System.currentTimeMillis());
assertRegistered(registration2, "http://localhost:8082", true);
// Just the debug flag should not unregister the User Agent.
registration1 = manager.configure("default1", "http://localhost:8081", false, System.currentTimeMillis());
assertRegistered(registration1, "http://localhost:8081", false);
-
- // But the configuration endpoint should unregister the correct User Agent.
+
+ // But the configuration endpoint should unregister the correct User Agent.
registration2 = manager.configure("default2", "http://localhost:8083", false, System.currentTimeMillis());
}
@Test
public void testSubscribeChannel() throws Exception {
manager.configure("default", "http://localhost:8080", false, System.currentTimeMillis());
PushRegistration registration = manager.registerUserAgent("default", System.currentTimeMillis());
assertRegistered(registration, "http://localhost:8080", false);
@@ -175,22 +165,16 @@ public class TestPushManager {
// We should be able to register with non-null serviceData.
final JSONObject webpushData = new JSONObject();
webpushData.put("version", 5);
PushSubscription subscription = manager.subscribeChannel("default", "webpush", webpushData, System.currentTimeMillis());
assertSubscribed(subscription);
// No exception is success.
manager.unsubscribeChannel(subscription.chid);
-
- // The actual unsubscribe request is made on the Gecko background thread; wait for it to
- // clear before verifying the push client was invoked.
- // Cribbed from http://stackoverflow.com/a/33928853.
- ((ShadowLooper) ShadowExtractor.extract(GeckoBackgroundThread.getHandler().getLooper())).idle();
- verify(pushClient).unsubscribeChannel(registration.uaid.value, registration.secret, subscription.chid);
}
public void testUnsubscribeUnknownChannel() throws Exception {
manager.configure("default", "http://localhost:8080", false, System.currentTimeMillis());
PushRegistration registration = manager.registerUserAgent("default", System.currentTimeMillis());
assertRegistered(registration, "http://localhost:8080", false);
doThrow(new RuntimeException())
@@ -198,52 +182,43 @@ public class TestPushManager {
.unsubscribeChannel(anyString(), anyString(), anyString());
// Un-subscribing from an unknown channel succeeds: we just ignore the request.
manager.unsubscribeChannel(UUID.randomUUID().toString());
}
@Test
public void testStartupBeforeConfiguration() throws Exception {
- verify(gcmTokenClient, never()).getToken(anyString());
+ verify(gcmTokenClient, never()).getToken(anyString(), anyBoolean());
manager.startup(System.currentTimeMillis());
- verify(gcmTokenClient, times(1)).getToken(PushManager.MOZ_ANDROID_GCM_SENDER_ID);
+ verify(gcmTokenClient, times(1)).getToken(AppConstants.MOZ_ANDROID_GCM_SENDERID, false);
}
@Test
public void testStartupBeforeRegistration() throws Exception {
PushRegistration registration = manager.configure("default", "http://localhost:8080", true, System.currentTimeMillis());
assertOnlyConfigured(registration, "http://localhost:8080", true);
manager.startup(System.currentTimeMillis());
- verify(gcmTokenClient, times(1)).getToken(anyString());
+ verify(gcmTokenClient, times(1)).getToken(anyString(), anyBoolean());
}
@Test
public void testStartupAfterRegistration() throws Exception {
PushRegistration registration = manager.configure("default", "http://localhost:8080", true, System.currentTimeMillis());
assertOnlyConfigured(registration, "http://localhost:8080", true);
registration = manager.registerUserAgent("default", System.currentTimeMillis());
assertRegistered(registration, "http://localhost:8080", true);
manager.startup(System.currentTimeMillis());
// Rather tautological.
PushRegistration updatedRegistration = manager.state.getRegistration("default");
Assert.assertEquals(registration.uaid, updatedRegistration.uaid);
-
- // Now, try well into the future. Our registration has expired, and we
- // have no subscriptions, so it should be dropped.
- long future = System.currentTimeMillis() + 2 * PushManager.TIME_BETWEEN_AUTOPUSH_UAID_REGISTRATION_IN_MILLIS;
- manager.startup(future);
-
- ((ShadowLooper) ShadowExtractor.extract(GeckoBackgroundThread.getHandler().getLooper())).idle();
- verify(pushClient).unregisterUserAgent(anyString(), anyString());
- Assert.assertTrue(state.registrations.isEmpty());
}
@Test
public void testStartupAfterSubscription() throws Exception {
PushRegistration registration = manager.configure("default", "http://localhost:8080", true, System.currentTimeMillis());
assertOnlyConfigured(registration, "http://localhost:8080", true);
registration = manager.registerUserAgent("default", System.currentTimeMillis());
@@ -253,19 +228,10 @@ public class TestPushManager {
assertSubscribed(subscription);
manager.startup(System.currentTimeMillis());
// Rather tautological.
registration = manager.registrationForSubscription(subscription.chid);
PushSubscription updatedSubscription = registration.getSubscription(subscription.chid);
Assert.assertEquals(subscription.chid, updatedSubscription.chid);
-
- // Now, try well into the future. Our registration has expired, but we
- // have subscriptions, so it shouldn't be dropped.
- long future = System.currentTimeMillis() + 2 * PushManager.TIME_BETWEEN_AUTOPUSH_UAID_REGISTRATION_IN_MILLIS;
- manager.startup(future);
-
- ((ShadowLooper) ShadowExtractor.extract(GeckoBackgroundThread.getHandler().getLooper())).idle();
- verify(pushClient, never()).unregisterUserAgent(anyString(), anyString());
- Assert.assertNotNull(manager.registrationForSubscription(subscription.chid));
}
}