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
This commit is contained in:
Greyson Parrelli 2018-11-13 00:28:46 -08:00
parent 446585ad68
commit e7c00a3066
3 changed files with 59 additions and 19 deletions

View File

@ -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.runAttempt = runAttempt;
}
@Override
protected void initialize(@NonNull SafeData data) {
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 { }
}

View File

@ -28,16 +28,18 @@ public class SmsSentJob extends MasterSecretJob {
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());
@ -45,6 +47,7 @@ public class SmsSentJob extends MasterSecretJob {
this.messageId = messageId;
this.action = action;
this.result = result;
this.runAttempt = runAttempt;
}
@Override
@ -52,6 +55,7 @@ public class SmsSentJob extends MasterSecretJob {
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);

View File

@ -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());