[Feed] After destroy() still call consumers, fix scheduler bridge.

Follow up to https://chromium-review.googlesource.com/c/chromium/src/+/1345422,
doing a couple of things.

1. Removing the destroy warning comment, as all bridges will need to follow
this pattern indefinitely. It is not a temporary comment, and adds significant
bulk to any bridge that may become out of sync over time.
2. Call consumers/callbacks immediately with error, null, or default responses.
This allows the Feed to take reasonable responses and not hang during shutdown.
3. Fix mistake in scheduler check that was backwards, causing the scheduler to
think it perpetually had an outstanding request.

Bug: 901414
Change-Id: Ied653541755d5fb1b204c4968e755364cdcece81
Reviewed-on: https://chromium-review.googlesource.com/c/1355614Reviewed-by: Filip Gorski <fgorski@chromium.org>
Commit-Queue: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612463}

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedContentStorage.java

@@ -51,48 +51,49 @@ public class FeedContentStorage implements ContentStorage {
 
     @Override
     public void get(List<String> keys, Consumer < Result < Map<String, byte[]>>> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedContentBridge == null) return;
-
-        mFeedContentBridge.loadContent(keys,
-                (Map<String, byte[]> data)
-                        -> consumer.accept(Result.success(data)),
-                (Void ignored) -> consumer.accept(Result.failure()));
+        if (mFeedContentBridge == null) {
+            consumer.accept(Result.failure());
+        } else {
+            mFeedContentBridge.loadContent(keys,
+                    (Map<String, byte[]> data)
+                            -> consumer.accept(Result.success(data)),
+                    (Void ignored) -> consumer.accept(Result.failure()));
+        }
     }
 
     @Override
     public void getAll(String prefix, Consumer < Result < Map<String, byte[]>>> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedContentBridge == null) return;
-
-        mFeedContentBridge.loadContentByPrefix(prefix,
-                (Map<String, byte[]> data)
-                        -> consumer.accept(Result.success(data)),
-                (Void ignored) -> consumer.accept(Result.failure()));
+        if (mFeedContentBridge == null) {
+            consumer.accept(Result.failure());
+        } else {
+            mFeedContentBridge.loadContentByPrefix(prefix,
+                    (Map<String, byte[]> data)
+                            -> consumer.accept(Result.success(data)),
+                    (Void ignored) -> consumer.accept(Result.failure()));
+        }
     }
 
     @Override
     public void commit(ContentMutation mutation, Consumer<CommitResult> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedContentBridge == null) return;
-
-        mFeedContentBridge.commitContentMutation(mutation,
-                (Boolean result)
-                        -> consumer.accept(result ? CommitResult.SUCCESS : CommitResult.FAILURE));
+        if (mFeedContentBridge == null) {
+            consumer.accept(CommitResult.FAILURE);
+        } else {
+            mFeedContentBridge.commitContentMutation(mutation,
+                    (Boolean result)
+                            -> consumer.accept(
+                                    result ? CommitResult.SUCCESS : CommitResult.FAILURE));
+        }
     }
 
     @Override
     public void getAllKeys(Consumer < Result < List<String>>> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedContentBridge == null) return;
-
-        mFeedContentBridge.loadAllContentKeys(
-                (String[] keys)
-                        -> consumer.accept(Result.success(Arrays.asList(keys))),
-                (Void ignored) -> consumer.accept(Result.failure()));
+        if (mFeedContentBridge == null) {
+            consumer.accept(Result.failure());
+        } else {
+            mFeedContentBridge.loadAllContentKeys(
+                    (String[] keys)
+                            -> consumer.accept(Result.success(Arrays.asList(keys))),
+                    (Void ignored) -> consumer.accept(Result.failure()));
+        }
     }
 }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedJournalStorage.java

@@ -50,59 +50,61 @@ public class FeedJournalStorage implements JournalStorage {
 
     @Override
     public void read(String journalName, Consumer < Result < List<byte[]>>> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedJournalBridge == null) return;
-
-        mFeedJournalBridge.loadJournal(journalName, (byte[][] entries) -> {
-            List<byte[]> journal = Arrays.asList(entries);
-            consumer.accept(Result.success(journal));
-        }, (Void ignored) -> consumer.accept(Result.failure()));
+        if (mFeedJournalBridge == null) {
+            consumer.accept(Result.failure());
+        } else {
+            mFeedJournalBridge.loadJournal(journalName, (byte[][] entries) -> {
+                List<byte[]> journal = Arrays.asList(entries);
+                consumer.accept(Result.success(journal));
+            }, (Void ignored) -> consumer.accept(Result.failure()));
+        }
     }
 
     @Override
     public void commit(JournalMutation mutation, Consumer<CommitResult> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedJournalBridge == null) return;
-
-        mFeedJournalBridge.commitJournalMutation(mutation,
-                (Boolean result)
-                        -> consumer.accept(result ? CommitResult.SUCCESS : CommitResult.FAILURE));
+        if (mFeedJournalBridge == null) {
+            consumer.accept(CommitResult.FAILURE);
+        } else {
+            mFeedJournalBridge.commitJournalMutation(mutation,
+                    (Boolean result)
+                            -> consumer.accept(
+                                    result ? CommitResult.SUCCESS : CommitResult.FAILURE));
+        }
     }
 
     @Override
     public void exists(String journalName, Consumer<Result<Boolean>> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedJournalBridge == null) return;
-
-        mFeedJournalBridge.doesJournalExist(journalName,
-                (Boolean exist)
-                        -> consumer.accept(Result.success(exist)),
-                (Void ignored) -> consumer.accept(Result.failure()));
+        if (mFeedJournalBridge == null) {
+            consumer.accept(Result.failure());
+        } else {
+            mFeedJournalBridge.doesJournalExist(journalName,
+                    (Boolean exist)
+                            -> consumer.accept(Result.success(exist)),
+                    (Void ignored) -> consumer.accept(Result.failure()));
+        }
     }
 
     @Override
     public void getAllJournals(Consumer < Result < List<String>>> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedJournalBridge == null) return;
-
-        mFeedJournalBridge.loadAllJournalKeys(
-                (String[] data)
-                        -> consumer.accept(Result.success(Arrays.asList(data))),
-                (Void ignored) -> consumer.accept(Result.failure()));
+        if (mFeedJournalBridge == null) {
+            consumer.accept(Result.failure());
+        } else {
+            mFeedJournalBridge.loadAllJournalKeys(
+                    (String[] data)
+                            -> consumer.accept(Result.success(Arrays.asList(data))),
+                    (Void ignored) -> consumer.accept(Result.failure()));
+        }
     }
 
     @Override
     public void deleteAll(Consumer<CommitResult> consumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mFeedJournalBridge == null) return;
-
-        mFeedJournalBridge.deleteAllJournals(
-                (Boolean result)
-                        -> consumer.accept(result ? CommitResult.SUCCESS : CommitResult.FAILURE));
+        if (mFeedJournalBridge == null) {
+            consumer.accept(CommitResult.FAILURE);
+        } else {
+            mFeedJournalBridge.deleteAllJournals(
+                    (Boolean result)
+                            -> consumer.accept(
+                                    result ? CommitResult.SUCCESS : CommitResult.FAILURE));
+        }
     }
 }

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedNetworkBridge.java

@@ -42,12 +42,13 @@ public class FeedNetworkBridge implements NetworkClient {
 
     @Override
     public void send(HttpRequest request, Consumer<HttpResponse> responseConsumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge == 0) return;
-
-        nativeSendNetworkRequest(mNativeBridge, request.getUri().toString(), request.getMethod(),
-                request.getBody(), result -> responseConsumer.accept(result));
+        if (mNativeBridge == 0) {
+            responseConsumer.accept(createHttpResponse(500, new byte[0]));
+        } else {
+            nativeSendNetworkRequest(mNativeBridge, request.getUri().toString(),
+                    request.getMethod(), request.getBody(),
+                    result -> responseConsumer.accept(result));
+        }
     }
 
     @Override

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedOfflineBridge.java

@@ -17,6 +17,7 @@ import org.chromium.chrome.browser.profiles.Profile;
 
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -57,66 +58,58 @@ public class FeedOfflineBridge
 
     @Override
     public Long getOfflineIdIfPageIsOfflined(String url) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge == 0) return 0L;
-
-        return (Long) nativeGetOfflineId(mNativeBridge, url);
+        if (mNativeBridge == 0) {
+            return 0L;
+        } else {
+            return (Long) nativeGetOfflineId(mNativeBridge, url);
+        }
     }
 
     @Override
     public void getOfflineStatus(
             List<String> urlsToRetrieve, Consumer<List<String>> urlListConsumer) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge == 0) return;
-
-        String[] urlsArray = urlsToRetrieve.toArray(new String[urlsToRetrieve.size()]);
-        nativeGetOfflineStatus(mNativeBridge, urlsArray,
-                (String[] urlsAsArray) -> urlListConsumer.accept(Arrays.asList(urlsAsArray)));
+        if (mNativeBridge == 0) {
+            urlListConsumer.accept(Collections.emptyList());
+        } else {
+            String[] urlsArray = urlsToRetrieve.toArray(new String[urlsToRetrieve.size()]);
+            nativeGetOfflineStatus(mNativeBridge, urlsArray,
+                    (String[] urlsAsArray) -> urlListConsumer.accept(Arrays.asList(urlsAsArray)));
+        }
     }
 
     @Override
     public void addOfflineStatusListener(OfflineStatusListener offlineStatusListener) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge == 0) return;
-
-        mListeners.add(offlineStatusListener);
+        if (mNativeBridge != 0) {
+            mListeners.add(offlineStatusListener);
+        }
     }
 
     @Override
     public void removeOfflineStatusListener(OfflineStatusListener offlineStatusListener) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge == 0) return;
-
-        mListeners.remove(offlineStatusListener);
-        if (mListeners.isEmpty()) {
-            nativeOnNoListeners(mNativeBridge);
+        if (mNativeBridge != 0) {
+            mListeners.remove(offlineStatusListener);
+            if (mListeners.isEmpty()) {
+                nativeOnNoListeners(mNativeBridge);
+            }
         }
     }
 
     @Override
     public void onContentRemoved(List<ContentRemoval> contentRemoved) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge == 0) return;
-
-        List<String> userDrivenRemovals = takeUserDriveRemovalsOnly(contentRemoved);
-        if (!userDrivenRemovals.isEmpty()) {
-            nativeOnContentRemoved(mNativeBridge,
-                    userDrivenRemovals.toArray(new String[userDrivenRemovals.size()]));
+        if (mNativeBridge != 0) {
+            List<String> userDrivenRemovals = takeUserDriveRemovalsOnly(contentRemoved);
+            if (!userDrivenRemovals.isEmpty()) {
+                nativeOnContentRemoved(mNativeBridge,
+                        userDrivenRemovals.toArray(new String[userDrivenRemovals.size()]));
+            }
         }
     }
 
     @Override
     public void onNewContentReceived(boolean isNewRefresh, long contentCreationDateTimeMs) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge == 0) return;
-
-        nativeOnNewContentReceived(mNativeBridge);
+        if (mNativeBridge != 0) {
+            nativeOnNewContentReceived(mNativeBridge);
+        }
     }
 
     /**
@@ -145,8 +138,6 @@ public class FeedOfflineBridge
     @CalledByNative
     private void getKnownContent() {
         mKnownContentApi.getKnownContent((List<ContentMetadata> metadataList) -> {
-            // Bridge could have been destroyed for policy when this is called.
-            // See https://crbug.com/901414.
             if (mNativeBridge == 0) return;
 
             for (ContentMetadata metadata : metadataList) {

chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/FeedSchedulerBridge.java

@@ -62,8 +62,6 @@ public class FeedSchedulerBridge implements FeedScheduler {
 
     @Override
     public int shouldSessionRequestData(SessionManagerState sessionManagerState) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
         if (mNativeBridge == 0) return SchedulerApi.RequestBehavior.UNKNOWN;
 
         @NativeRequestBehavior
@@ -95,20 +93,16 @@ public class FeedSchedulerBridge implements FeedScheduler {
 
     @Override
     public void onReceiveNewContent(long contentCreationDateTimeMs) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge != 0) return;
-
-        nativeOnReceiveNewContent(mNativeBridge, contentCreationDateTimeMs);
+        if (mNativeBridge != 0) {
+            nativeOnReceiveNewContent(mNativeBridge, contentCreationDateTimeMs);
+        }
     }
 
     @Override
     public void onRequestError(int networkResponseCode) {
-        // Bridge could have been destroyed for policy when this is called.
-        // See https://crbug.com/901414.
-        if (mNativeBridge != 0) return;
-
-        nativeOnRequestError(mNativeBridge, networkResponseCode);
+        if (mNativeBridge != 0) {
+            nativeOnRequestError(mNativeBridge, networkResponseCode);
+        }
     }
 
     @Override