From e7c00a30662f23251cbdea1381568a15a528b8ae Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Tue, 13 Nov 2018 00:28:46 -0800 Subject: [PATCH] Fix issue where we may oversend SMS messages. Because SMS sending is split over two jobs, there's no max retry limit respected if we find out about the failure in SmsSentJob -- it's requeued as a new job with a fresh attempt counter. This commit carries a retry count between the two jobs. It also verifies that we have service before attempting to send a message at all. Relates to #8268 --- .../securesms/jobs/SmsSendJob.java | 46 ++++++++++++++++--- .../securesms/jobs/SmsSentJob.java | 27 ++++++----- .../service/SmsDeliveryListener.java | 5 +- 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/src/org/thoughtcrime/securesms/jobs/SmsSendJob.java b/src/org/thoughtcrime/securesms/jobs/SmsSendJob.java index 9600646b39..7a5019ed8e 100644 --- a/src/org/thoughtcrime/securesms/jobs/SmsSendJob.java +++ b/src/org/thoughtcrime/securesms/jobs/SmsSendJob.java @@ -10,6 +10,8 @@ import android.telephony.PhoneNumberUtils; import android.telephony.SmsManager; import org.thoughtcrime.securesms.jobmanager.SafeData; +import org.thoughtcrime.securesms.jobs.requirements.NetworkOrServiceRequirement; +import org.thoughtcrime.securesms.jobs.requirements.ServiceRequirement; import org.thoughtcrime.securesms.logging.Log; import org.thoughtcrime.securesms.crypto.MasterSecret; @@ -20,6 +22,7 @@ import org.thoughtcrime.securesms.database.model.SmsMessageRecord; import org.thoughtcrime.securesms.notifications.MessageNotifier; import org.thoughtcrime.securesms.recipients.Recipient; import org.thoughtcrime.securesms.service.SmsDeliveryListener; +import org.thoughtcrime.securesms.transport.RetryLaterException; import org.thoughtcrime.securesms.transport.UndeliverableMessageException; import org.thoughtcrime.securesms.util.NumberUtil; import org.thoughtcrime.securesms.util.TextSecurePreferences; @@ -33,27 +36,38 @@ public class SmsSendJob extends SendJob { private static final long serialVersionUID = -5118520036244759718L; private static final String TAG = SmsSendJob.class.getSimpleName(); + private static final int MAX_ATTEMPTS = 15; private static final String KEY_MESSAGE_ID = "message_id"; + private static final String KEY_RUN_ATTEMPT = "run_attempt"; private long messageId; + private int runAttempt; public SmsSendJob() { super(null, null); } public SmsSendJob(Context context, long messageId, String name) { + this(context, messageId, name, 0); + } + + public SmsSendJob(Context context, long messageId, String name, int runAttempt) { super(context, constructParameters(name)); - this.messageId = messageId; + this.messageId = messageId; + this.runAttempt = runAttempt; } @Override protected void initialize(@NonNull SafeData data) { - messageId = data.getLong(KEY_MESSAGE_ID); + messageId = data.getLong(KEY_MESSAGE_ID); + runAttempt = data.getInt(KEY_RUN_ATTEMPT); } @Override protected @NonNull Data serialize(@NonNull Data.Builder dataBuilder) { - return dataBuilder.putLong(KEY_MESSAGE_ID, messageId).build(); + return dataBuilder.putLong(KEY_MESSAGE_ID, messageId) + .putInt(KEY_RUN_ATTEMPT, runAttempt) + .build(); } @Override @@ -62,12 +76,22 @@ public class SmsSendJob extends SendJob { } @Override - public void onSend(MasterSecret masterSecret) throws NoSuchMessageException { + public void onSend(MasterSecret masterSecret) throws NoSuchMessageException, RequirementNotMetException, TooManyRetriesException { + if (!requirementsMet()) { + Log.w(TAG, "No service. Retrying."); + throw new RequirementNotMetException(); + } + + if (runAttempt >= MAX_ATTEMPTS) { + Log.w(TAG, "Hit the retry limit. Failing."); + throw new TooManyRetriesException(); + } + SmsDatabase database = DatabaseFactory.getSmsDatabase(context); SmsMessageRecord record = database.getMessage(messageId); try { - Log.i(TAG, "Sending message: " + messageId); + Log.i(TAG, "Sending message: " + messageId + " (attempt " + runAttempt + ")"); deliver(record); Log.i(TAG, "Sent message: " + messageId); } catch (UndeliverableMessageException ude) { @@ -95,6 +119,14 @@ public class SmsSendJob extends SendJob { } } + private boolean requirementsMet() { + if (TextSecurePreferences.isWifiSmsEnabled(context)) { + return new NetworkOrServiceRequirement(context).isPresent(); + } else { + return new ServiceRequirement(context).isPresent(); + } + } + private void deliver(SmsMessageRecord message) throws UndeliverableMessageException { @@ -186,6 +218,7 @@ public class SmsSendJob extends SendJob { pending.putExtra("type", type); pending.putExtra("message_id", messageId); + pending.putExtra("run_attempt", Math.max(runAttempt, getRunAttemptCount())); pending.putExtra("upgraded", upgraded); pending.putExtra("push", push); @@ -213,10 +246,11 @@ public class SmsSendJob extends SendJob { private static JobParameters constructParameters(String name) { JobParameters.Builder builder = JobParameters.newBuilder() .withMasterSecretRequirement() - .withRetryCount(15) + .withRetryCount(MAX_ATTEMPTS) .withGroupId(name); return builder.create(); } + private static class TooManyRetriesException extends Exception { } } diff --git a/src/org/thoughtcrime/securesms/jobs/SmsSentJob.java b/src/org/thoughtcrime/securesms/jobs/SmsSentJob.java index b335022448..a34e3639fc 100644 --- a/src/org/thoughtcrime/securesms/jobs/SmsSentJob.java +++ b/src/org/thoughtcrime/securesms/jobs/SmsSentJob.java @@ -25,33 +25,37 @@ public class SmsSentJob extends MasterSecretJob { private static final long serialVersionUID = -2624694558755317560L; private static final String TAG = SmsSentJob.class.getSimpleName(); - private static final String KEY_MESSAGE_ID = "message_id"; - private static final String KEY_ACTION = "action"; - private static final String KEY_RESULT = "result"; + private static final String KEY_MESSAGE_ID = "message_id"; + private static final String KEY_ACTION = "action"; + private static final String KEY_RESULT = "result"; + private static final String KEY_RUN_ATTEMPT = "run_attempt"; private long messageId; private String action; private int result; + private int runAttempt; public SmsSentJob() { super(null, null); } - public SmsSentJob(Context context, long messageId, String action, int result) { + public SmsSentJob(Context context, long messageId, String action, int result, int runAttempt) { super(context, JobParameters.newBuilder() .withMasterSecretRequirement() .create()); - this.messageId = messageId; - this.action = action; - this.result = result; + this.messageId = messageId; + this.action = action; + this.result = result; + this.runAttempt = runAttempt; } @Override protected void initialize(@NonNull SafeData data) { - messageId = data.getLong(KEY_MESSAGE_ID); - action = data.getString(KEY_ACTION); - result = data.getInt(KEY_RESULT); + messageId = data.getLong(KEY_MESSAGE_ID); + action = data.getString(KEY_ACTION); + result = data.getInt(KEY_RESULT); + runAttempt = data.getInt(KEY_RUN_ATTEMPT); } @Override @@ -59,6 +63,7 @@ public class SmsSentJob extends MasterSecretJob { return dataBuilder.putLong(KEY_MESSAGE_ID, messageId) .putString(KEY_ACTION, action) .putInt(KEY_RESULT, result) + .putInt(KEY_RUN_ATTEMPT, runAttempt) .build(); } @@ -104,7 +109,7 @@ public class SmsSentJob extends MasterSecretJob { Log.w(TAG, "Service connectivity problem, requeuing..."); ApplicationContext.getInstance(context) .getJobManager() - .add(new SmsSendJob(context, messageId, record.getIndividualRecipient().getAddress().serialize())); + .add(new SmsSendJob(context, messageId, record.getIndividualRecipient().getAddress().serialize(), runAttempt + 1)); break; default: database.markAsSentFailed(messageId); diff --git a/src/org/thoughtcrime/securesms/service/SmsDeliveryListener.java b/src/org/thoughtcrime/securesms/service/SmsDeliveryListener.java index 62b6714a75..09047f2617 100644 --- a/src/org/thoughtcrime/securesms/service/SmsDeliveryListener.java +++ b/src/org/thoughtcrime/securesms/service/SmsDeliveryListener.java @@ -22,12 +22,13 @@ public class SmsDeliveryListener extends BroadcastReceiver { public void onReceive(Context context, Intent intent) { JobManager jobManager = ApplicationContext.getInstance(context).getJobManager(); long messageId = intent.getLongExtra("message_id", -1); + int runAttempt = intent.getIntExtra("run_attempt", 0); switch (intent.getAction()) { case SENT_SMS_ACTION: int result = getResultCode(); - jobManager.add(new SmsSentJob(context, messageId, SENT_SMS_ACTION, result)); + jobManager.add(new SmsSentJob(context, messageId, SENT_SMS_ACTION, result, runAttempt)); break; case DELIVERED_SMS_ACTION: byte[] pdu = intent.getByteArrayExtra("pdu"); @@ -58,7 +59,7 @@ public class SmsDeliveryListener extends BroadcastReceiver { else if (status >> 24 == 3) status = SmsDatabase.Status.STATUS_FAILED; } - jobManager.add(new SmsSentJob(context, messageId, DELIVERED_SMS_ACTION, status)); + jobManager.add(new SmsSentJob(context, messageId, DELIVERED_SMS_ACTION, status, runAttempt)); break; default: Log.w(TAG, "Unknown action: " + intent.getAction());