Merge pull request #3294 from mauriciocolli/fix-network-issues-detection

Fix detection of network related exceptions

app/src/main/java/org/schabi/newpipe/App.java

@@ -27,7 +27,7 @@ import org.schabi.newpipe.report.AcraReportSenderFactory;
 import org.schabi.newpipe.report.ErrorActivity;
 import org.schabi.newpipe.report.UserAction;
 import org.schabi.newpipe.settings.SettingsActivity;
-import org.schabi.newpipe.util.ExtractorHelper;
+import org.schabi.newpipe.util.ExceptionUtils;
 import org.schabi.newpipe.util.Localization;
 import org.schabi.newpipe.util.ServiceHelper;
 import org.schabi.newpipe.util.StateSaver;
@@ -173,7 +173,7 @@ public class App extends Application {
 
             private boolean isThrowableIgnored(@NonNull final Throwable throwable) {
                 // Don't crash the application over a simple network problem
-                return ExtractorHelper.hasAssignableCauseThrowable(throwable,
+                return ExceptionUtils.hasAssignableCause(throwable,
                         // network api cancellation
                         IOException.class, SocketException.class,
                         // blocking code disposed
@@ -182,7 +182,7 @@ public class App extends Application {
 
             private boolean isThrowableCritical(@NonNull final Throwable throwable) {
                 // Though these exceptions cannot be ignored
-                return ExtractorHelper.hasAssignableCauseThrowable(throwable,
+                return ExceptionUtils.hasAssignableCause(throwable,
                         NullPointerException.class, IllegalArgumentException.class, // bug in app
                         OnErrorNotImplementedException.class, MissingBackpressureException.class,
                         IllegalStateException.class); // bug in operator

app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java

@@ -24,10 +24,9 @@ import org.schabi.newpipe.extractor.exceptions.ContentNotSupportedException;
 import org.schabi.newpipe.extractor.exceptions.ReCaptchaException;
 import org.schabi.newpipe.report.ErrorActivity;
 import org.schabi.newpipe.report.UserAction;
-import org.schabi.newpipe.util.ExtractorHelper;
+import org.schabi.newpipe.util.ExceptionUtils;
 import org.schabi.newpipe.util.InfoCache;
 
-import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
@@ -201,7 +200,7 @@ public abstract class BaseStateFragment<I> extends BaseFragment implements ViewC
             return true;
         }
 
-        if (ExtractorHelper.isInterruptedCaused(exception)) {
+        if (ExceptionUtils.isInterruptedCaused(exception)) {
             if (DEBUG) {
                 Log.w(TAG, "onError() isInterruptedCaused! = [" + exception + "]");
             }
@@ -214,7 +213,7 @@ public abstract class BaseStateFragment<I> extends BaseFragment implements ViewC
         } else if (exception instanceof ContentNotAvailableException) {
             showError(getString(R.string.content_not_available), false);
             return true;
-        } else if (exception instanceof IOException) {
+        } else if (ExceptionUtils.isNetworkRelated(exception)) {
             showError(getString(R.string.network_error), true);
             return true;
         } else if (exception instanceof ContentNotSupportedException) {

app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java

@@ -53,9 +53,6 @@ import org.schabi.newpipe.util.FireTvUtils;
 import org.schabi.newpipe.util.NavigationHelper;
 import org.schabi.newpipe.util.ServiceHelper;
 
-import java.io.IOException;
-import java.io.InterruptedIOException;
-import java.net.SocketException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -770,12 +767,7 @@ public class SearchFragment extends BaseListFragment<SearchInfo, ListExtractor.I
                     if (listNotification.isOnNext()) {
                         handleSuggestions(listNotification.getValue());
                     } else if (listNotification.isOnError()) {
-                        Throwable error = listNotification.getError();
+                        onSuggestionError(listNotification.getError());
-                        if (!ExtractorHelper.hasAssignableCauseThrowable(error,
-                                IOException.class, SocketException.class,
-                                InterruptedException.class, InterruptedIOException.class)) {
-                            onSuggestionError(error);
-                        }
                     }
                 });
     }

app/src/main/java/org/schabi/newpipe/local/feed/service/FeedLoadService.kt

@@ -52,6 +52,7 @@ import org.schabi.newpipe.local.feed.FeedDatabaseManager
 import org.schabi.newpipe.local.feed.service.FeedEventManager.Event.*
 import org.schabi.newpipe.local.feed.service.FeedEventManager.postEvent
 import org.schabi.newpipe.local.subscription.SubscriptionManager
+import org.schabi.newpipe.util.ExceptionUtils
 import org.schabi.newpipe.util.ExtractorHelper
 import java.io.IOException
 import java.util.*
@@ -333,11 +334,12 @@ class FeedLoadService : Service() {
                 val cause = error.cause
 
                 when {
-                    error is IOException -> throw error
-                    cause is IOException -> throw cause
-
                     error is ReCaptchaException -> throw error
                     cause is ReCaptchaException -> throw cause
+
+                    error is IOException -> throw error
+                    cause is IOException -> throw cause
+                    ExceptionUtils.isNetworkRelated(error) -> throw IOException(error)
                 }
             }
         }

app/src/main/java/org/schabi/newpipe/local/subscription/services/BaseImportExportService.java

@@ -38,9 +38,9 @@ import org.schabi.newpipe.extractor.subscription.SubscriptionExtractor;
 import org.schabi.newpipe.local.subscription.SubscriptionManager;
 import org.schabi.newpipe.report.ErrorActivity;
 import org.schabi.newpipe.report.UserAction;
+import org.schabi.newpipe.util.ExceptionUtils;
 
 import java.io.FileNotFoundException;
-import java.io.IOException;
 import java.util.Collections;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -227,7 +227,7 @@ public abstract class BaseImportExportService extends Service {
             message = getString(R.string.invalid_source);
         } else if (error instanceof FileNotFoundException) {
             message = getString(R.string.invalid_file);
-        } else if (error instanceof IOException) {
+        } else if (ExceptionUtils.isNetworkRelated(error)) {
             message = getString(R.string.network_error);
         }
         return message;

app/src/main/java/org/schabi/newpipe/local/subscription/services/SubscriptionsImportService.java

@@ -35,6 +35,7 @@ import org.schabi.newpipe.extractor.NewPipe;
 import org.schabi.newpipe.extractor.channel.ChannelInfo;
 import org.schabi.newpipe.extractor.subscription.SubscriptionItem;
 import org.schabi.newpipe.util.Constants;
+import org.schabi.newpipe.util.ExceptionUtils;
 import org.schabi.newpipe.util.ExtractorHelper;
 
 import java.io.File;
@@ -245,8 +246,10 @@ public class SubscriptionsImportService extends BaseImportExportService {
                 final Throwable cause = error.getCause();
                 if (error instanceof IOException) {
                     throw (IOException) error;
-                } else if (cause != null && cause instanceof IOException) {
+                } else if (cause instanceof IOException) {
                     throw (IOException) cause;
+                } else if (ExceptionUtils.isNetworkRelated(error)) {
+                    throw new IOException(error);
                 }
 
                 eventListener.onItemCompleted("");

app/src/main/java/org/schabi/newpipe/util/ExceptionUtils.kt

@@ -0,0 +1,82 @@
+package org.schabi.newpipe.util
+
+import java.io.IOException
+import java.io.InterruptedIOException
+
+class ExceptionUtils {
+    companion object {
+        /**
+         * @return if throwable is related to Interrupted exceptions, or one of its causes is.
+         */
+        @JvmStatic
+        fun isInterruptedCaused(throwable: Throwable): Boolean {
+            return hasExactCause(throwable,
+                    InterruptedIOException::class.java,
+                    InterruptedException::class.java)
+        }
+
+        /**
+         * @return if throwable is related to network issues, or one of its causes is.
+         */
+        @JvmStatic
+        fun isNetworkRelated(throwable: Throwable): Boolean {
+            return hasAssignableCause(throwable,
+                    IOException::class.java)
+        }
+
+        /**
+         * Calls [hasCause] with the `checkSubtypes` parameter set to false.
+         */
+        @JvmStatic
+        fun hasExactCause(throwable: Throwable, vararg causesToCheck: Class<*>): Boolean {
+            return hasCause(throwable, false, *causesToCheck)
+        }
+
+        /**
+         * Calls [hasCause] with the `checkSubtypes` parameter set to true.
+         */
+        @JvmStatic
+        fun hasAssignableCause(throwable: Throwable?, vararg causesToCheck: Class<*>): Boolean {
+            return hasCause(throwable, true, *causesToCheck)
+        }
+
+        /**
+         * Check if throwable has some cause from the causes to check, or is itself in it.
+         *
+         * If `checkIfAssignable` is true, not only the exact type will be considered equals, but also its subtypes.
+         *
+         * @param throwable throwable that will be checked.
+         * @param checkSubtypes if subtypes are also checked.
+         * @param causesToCheck an array of causes to check.
+         *
+         * @see Class.isAssignableFrom
+         */
+        @JvmStatic
+        tailrec fun hasCause(throwable: Throwable?, checkSubtypes: Boolean, vararg causesToCheck: Class<*>): Boolean {
+            if (throwable == null) {
+                return false
+            }
+
+            // Check if throwable is a subtype of any of the causes to check
+            causesToCheck.forEach { causeClass ->
+                if (checkSubtypes) {
+                    if (causeClass.isAssignableFrom(throwable.javaClass)) {
+                        return true
+                    }
+                } else {
+                    if (causeClass == throwable.javaClass) {
+                        return true
+                    }
+                }
+            }
+
+            val currentCause: Throwable? = throwable.cause
+            // Check if cause is not pointing to the same instance, to avoid infinite loops.
+            if (throwable !== currentCause) {
+                return hasCause(currentCause, checkSubtypes, *causesToCheck)
+            }
+
+            return false
+        }
+    }
+}

app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java

@@ -52,8 +52,6 @@ import org.schabi.newpipe.extractor.suggestion.SuggestionExtractor;
 import org.schabi.newpipe.report.ErrorActivity;
 import org.schabi.newpipe.report.UserAction;
 
-import java.io.IOException;
-import java.io.InterruptedIOException;
 import java.util.Collections;
 import java.util.List;
 
@@ -290,7 +288,7 @@ public final class ExtractorHelper {
                 Intent intent = new Intent(context, ReCaptchaActivity.class);
                 intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
                 context.startActivity(intent);
-            } else if (exception instanceof IOException) {
+            } else if (ExceptionUtils.isNetworkRelated(exception)) {
                 Toast.makeText(context, R.string.network_error, Toast.LENGTH_LONG).show();
             } else if (exception instanceof ContentNotAvailableException) {
                 Toast.makeText(context, R.string.content_not_available, Toast.LENGTH_LONG).show();
@@ -309,85 +307,4 @@ public final class ExtractorHelper {
             }
         });
     }
-
-    /**
-     * Check if throwable have the cause that can be assignable from the causes to check.
-     *
-     * @see Class#isAssignableFrom(Class)
-     * @param throwable     the throwable to be checked
-     * @param causesToCheck the causes to check
-     * @return whether the exception is an instance of a subclass of one of the causes
-     * or is caused by an instance of a subclass of one of the causes
-     */
-    public static boolean hasAssignableCauseThrowable(final Throwable throwable,
-                                                      final Class<?>... causesToCheck) {
-        // Check if getCause is not the same as cause (the getCause is already the root),
-        // as it will cause a infinite loop if it is
-        Throwable cause;
-        Throwable getCause = throwable;
-
-        // Check if throwable is a subclass of any of the filtered classes
-        final Class throwableClass = throwable.getClass();
-        for (Class<?> causesEl : causesToCheck) {
-            if (causesEl.isAssignableFrom(throwableClass)) {
-                return true;
-            }
-        }
-
-        // Iteratively checks if the root cause of the throwable is a subclass of the filtered class
-        while ((cause = throwable.getCause()) != null && getCause != cause) {
-            getCause = cause;
-            final Class causeClass = cause.getClass();
-            for (Class<?> causesEl : causesToCheck) {
-                if (causesEl.isAssignableFrom(causeClass)) {
-                    return true;
-                }
-            }
-        }
-        return false;
-    }
-
-    /**
-     * Check if throwable have the exact cause from one of the causes to check.
-     *
-     * @param throwable     the throwable to be checked
-     * @param causesToCheck the causes to check
-     * @return whether the exception is an instance of one of the causes
-     * or is caused by an instance of one of the causes
-     */
-    public static boolean hasExactCauseThrowable(final Throwable throwable,
-                                                 final Class<?>... causesToCheck) {
-        // Check if getCause is not the same as cause (the getCause is already the root),
-        // as it will cause a infinite loop if it is
-        Throwable cause;
-        Throwable getCause = throwable;
-
-        for (Class<?> causesEl : causesToCheck) {
-            if (throwable.getClass().equals(causesEl)) {
-                return true;
-            }
-        }
-
-        while ((cause = throwable.getCause()) != null && getCause != cause) {
-            getCause = cause;
-            for (Class<?> causesEl : causesToCheck) {
-                if (cause.getClass().equals(causesEl)) {
-                    return true;
-                }
-            }
-        }
-        return false;
-    }
-
-    /**
-     * Check if throwable have Interrupted* exception as one of its causes.
-     *
-     * @param throwable the throwable to be checkes
-     * @return whether the throwable is caused by an interruption
-     */
-    public static boolean isInterruptedCaused(final Throwable throwable) {
-        return ExtractorHelper.hasExactCauseThrowable(throwable,
-                InterruptedIOException.class,
-                InterruptedException.class);
-    }
 }

app/src/test/java/org/schabi/newpipe/util/ExceptionUtilsTest.kt

@@ -0,0 +1,69 @@
+package org.schabi.newpipe.util
+
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertTrue
+import org.junit.Test
+import org.schabi.newpipe.util.ExceptionUtils.Companion.hasAssignableCause
+import org.schabi.newpipe.util.ExceptionUtils.Companion.hasExactCause
+import java.io.IOException
+import java.io.InterruptedIOException
+import java.net.SocketException
+import javax.net.ssl.SSLException
+
+class ExceptionUtilsTest {
+    @Test fun `assignable causes`() {
+        assertTrue(hasAssignableCause(Throwable(), Throwable::class.java))
+        assertTrue(hasAssignableCause(Exception(), Exception::class.java))
+        assertTrue(hasAssignableCause(IOException(), Exception::class.java))
+
+        assertTrue(hasAssignableCause(IOException(), IOException::class.java))
+        assertTrue(hasAssignableCause(Exception(SocketException()), IOException::class.java))
+        assertTrue(hasAssignableCause(Exception(IllegalStateException()), RuntimeException::class.java))
+        assertTrue(hasAssignableCause(Exception(Exception(IOException())), IOException::class.java))
+        assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(IOException()))), IOException::class.java))
+        assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(SocketException()))), IOException::class.java))
+        assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(SSLException("IO")))), IOException::class.java))
+        assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), IOException::class.java))
+        assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), RuntimeException::class.java))
+
+        assertTrue(hasAssignableCause(IllegalStateException(), Throwable::class.java))
+        assertTrue(hasAssignableCause(IllegalStateException(), Exception::class.java))
+        assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), InterruptedIOException::class.java))
+    }
+
+    @Test fun `no assignable causes`() {
+        assertFalse(hasAssignableCause(Throwable(), Exception::class.java))
+        assertFalse(hasAssignableCause(Exception(), IOException::class.java))
+        assertFalse(hasAssignableCause(Exception(IllegalStateException()), IOException::class.java))
+        assertFalse(hasAssignableCause(Exception(NullPointerException()), IOException::class.java))
+        assertFalse(hasAssignableCause(Exception(IllegalStateException(Exception(Exception()))), IOException::class.java))
+        assertFalse(hasAssignableCause(Exception(IllegalStateException(Exception(SocketException()))), InterruptedIOException::class.java))
+        assertFalse(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), InterruptedException::class.java))
+    }
+
+    @Test fun `exact causes`() {
+        assertTrue(hasExactCause(Throwable(), Throwable::class.java))
+        assertTrue(hasExactCause(Exception(), Exception::class.java))
+
+        assertTrue(hasExactCause(IOException(), IOException::class.java))
+        assertTrue(hasExactCause(Exception(SocketException()), SocketException::class.java))
+        assertTrue(hasExactCause(Exception(Exception(IOException())), IOException::class.java))
+        assertTrue(hasExactCause(Exception(IllegalStateException(Exception(IOException()))), IOException::class.java))
+        assertTrue(hasExactCause(Exception(IllegalStateException(Exception(SocketException()))), SocketException::class.java))
+        assertTrue(hasExactCause(Exception(IllegalStateException(Exception(SSLException("IO")))), SSLException::class.java))
+        assertTrue(hasExactCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), InterruptedIOException::class.java))
+        assertTrue(hasExactCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), IllegalStateException::class.java))
+    }
+
+    @Test fun `no exact causes`() {
+        assertFalse(hasExactCause(Throwable(), Exception::class.java))
+        assertFalse(hasExactCause(Exception(), Throwable::class.java))
+
+        assertFalse(hasExactCause(SocketException(), IOException::class.java))
+        assertFalse(hasExactCause(IllegalStateException(), RuntimeException::class.java))
+        assertFalse(hasExactCause(Exception(SocketException()), IOException::class.java))
+        assertFalse(hasExactCause(Exception(IllegalStateException(Exception(IOException()))), RuntimeException::class.java))
+        assertFalse(hasExactCause(Exception(IllegalStateException(Exception(SocketException()))), IOException::class.java))
+        assertFalse(hasExactCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), IOException::class.java))
+    }
+}