Merge remote-tracking branch 'origin/trunk' into issue/rs-comments-subheaders

Author: nbradbury

WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadNotifier.java

@@ -11,6 +11,7 @@
 import androidx.annotation.Nullable;
 import androidx.collection.SparseArrayCompat;
 import androidx.core.app.NotificationCompat;
+import androidx.core.app.ServiceCompat;
 import androidx.core.app.TaskStackBuilder;
 
 import org.greenrobot.eventbus.EventBus;
@@ -128,9 +129,7 @@ private void updateNotificationBuilder(@Nullable PostImmutableModel post) {
     }
 
     private synchronized void startOrUpdateForegroundNotification(@Nullable PostImmutableModel post) {
-        boolean isTotalPostsAndMediaItemsCountZero = sNotificationData.mTotalPostItems == 0
-                                                     && sNotificationData.mTotalMediaItems == 0;
-        if (isTotalPostsAndMediaItemsCountZero) {
+        if (hasNoForegroundItemsTracked()) {
             return;
         }
         updateNotificationBuilder(post);
@@ -214,6 +213,12 @@ void incrementUploadedPostCountFromForegroundNotification(@NonNull PostImmutable
     }
 
     void incrementUploadedPostCountFromForegroundNotification(@NonNull PostImmutableModel post, boolean force) {
+        // Ignore stale completion callbacks that arrive after the foreground state was reset (e.g.
+        // cancellations dispatched during a foreground-service timeout). With nothing being tracked,
+        // counting them would push the current counters past the zeroed totals and corrupt the state.
+        if (hasNoForegroundItemsTracked()) {
+            return;
+        }
         // first we need to check that we only count this post once as "ended" (either successfully or with error)
         // for every error we get. We'll then try to increment the Post count as it's been cancelled/failed because the
         // related media was cancelled or has failed too (i.e. we can't upload a Post with failed media, therefore
@@ -232,6 +237,11 @@ void incrementUploadedPostCountFromForegroundNotification(@NonNull PostImmutable
     }
 
     void incrementUploadedMediaCountFromProgressNotification(int mediaId) {
+        // Ignore stale completion callbacks that arrive after the foreground state was reset (see
+        // incrementUploadedPostCountFromForegroundNotification above).
+        if (hasNoForegroundItemsTracked()) {
+            return;
+        }
         sNotificationData.mCurrentMediaItem++;
         if (!removeNotificationAndStopForegroundServiceIfNoItemsInQueue()) {
             // update Notification now
@@ -242,16 +252,32 @@ void incrementUploadedMediaCountFromProgressNotification(int mediaId) {
     private boolean removeNotificationAndStopForegroundServiceIfNoItemsInQueue() {
         if (sNotificationData.mCurrentPostItem == sNotificationData.mTotalPostItems
             && sNotificationData.mCurrentMediaItem == sNotificationData.mTotalMediaItems) {
-            mNotificationManager.cancel(sNotificationData.mNotificationId);
-            // reset the notification id so a new one is generated next time the service is started
-            sNotificationData.mNotificationId = 0;
-            resetNotificationCounters();
-            mService.stopForeground(true);
+            resetForegroundNotificationState();
+            ServiceCompat.stopForeground(mService, ServiceCompat.STOP_FOREGROUND_REMOVE);
             return true;
         }
         return false;
     }
 
+    private boolean hasNoForegroundItemsTracked() {
+        return sNotificationData.mTotalPostItems == 0 && sNotificationData.mTotalMediaItems == 0;
+    }
+
+    /**
+     * Clears the outstanding foreground notification and resets its state. Called both when all
+     * uploads complete and when the service is force-stopped (e.g. a foreground-service timeout),
+     * so the next upload starts a fresh foreground notification via startForeground().
+     */
+    void resetForegroundNotificationState() {
+        if (sNotificationData.mNotificationId != 0) {
+            // cancel the outstanding notification and reset the id so a new one is generated next
+            // time the service is started
+            mNotificationManager.cancel(sNotificationData.mNotificationId);
+            sNotificationData.mNotificationId = 0;
+        }
+        resetNotificationCounters();
+    }
+
     private void resetNotificationCounters() {
         sNotificationData.mCurrentPostItem = 0;
         sNotificationData.mCurrentMediaItem = 0;

WordPress/src/main/java/org/wordpress/android/ui/uploads/UploadService.java

@@ -9,6 +9,7 @@
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.RequiresApi;
+import androidx.core.app.ServiceCompat;
 
 import org.greenrobot.eventbus.EventBus;
 import org.greenrobot.eventbus.Subscribe;
@@ -176,26 +177,45 @@ public int onStartCommand(Intent intent, int flags, int startId) {
     @Override
     public void onTimeout(int startId) {
         super.onTimeout(startId);
-        stopSelf(startId);
-        AppLog.i(T.MAIN, "UploadService > timed out");
+        handleForegroundServiceTimeout();
     }
 
     // Android 15+ enforces a 6-hour-per-24h cap on dataSync foreground services. The system calls
-    // this two-arg overload before throwing ForegroundServiceDidNotStopInTimeException; we have a
-    // few seconds to stop. Cancel in-flight uploads eagerly so onDestroy() finishes within budget.
+    // this two-arg overload before throwing ForegroundServiceDidNotStopInTimeException.
     @Override
     @RequiresApi(Build.VERSION_CODES.VANILLA_ICE_CREAM)
     public void onTimeout(int startId, int fgsType) {
         super.onTimeout(startId, fgsType);
-        AppLog.w(T.MAIN, "UploadService > foreground service timeout reached (type=" + fgsType
-                         + "); stopping service");
+        AppLog.w(T.MAIN, "UploadService > foreground service timeout reached (type=" + fgsType + ")");
+        handleForegroundServiceTimeout();
+    }
+
+    /**
+     * Stops the service after the system signals a foreground-service timeout, in the few seconds we
+     * have before {@link android.app.RemoteServiceException.ForegroundServiceDidNotStopInTimeException}
+     * is thrown.
+     *
+     * We intentionally do NOT use {@code stopSelf(startId)} here: that overload only stops the service
+     * when {@code startId} matches the most recent start, but the service is started once per queued
+     * upload, so the id delivered to onTimeout() is usually stale and the call would be a no-op. We
+     * also leave the foreground state right away rather than relying on onDestroy() finishing within
+     * the timeout budget.
+     */
+    private void handleForegroundServiceTimeout() {
         if (mMediaUploadHandler != null) {
             mMediaUploadHandler.cancelInProgressUploads();
         }
         if (mPostUploadHandler != null) {
             mPostUploadHandler.cancelInProgressUploads();
         }
-        stopSelf(startId);
+        // Leave the foreground state immediately so the system's timeout window is satisfied, and
+        // reset the notifier so the next upload re-enters the foreground via startForeground().
+        if (mPostUploadNotifier != null) {
+            mPostUploadNotifier.resetForegroundNotificationState();
+        }
+        ServiceCompat.stopForeground(this, ServiceCompat.STOP_FOREGROUND_REMOVE);
+        stopSelf();
+        AppLog.i(T.MAIN, "UploadService > stopped after foreground service timeout");
     }
 
     private void unpackMediaIntent(@NonNull Intent intent) {