From 696f60fd38238bb1fa82a8d5be242557cf1dbd15 Mon Sep 17 00:00:00 2001 From: baka kaba Date: Sat, 13 Apr 2019 00:02:21 +0100 Subject: [PATCH 1/3] Fix network request errors when probated or logged out When I rewrote AwfulRequest I forgot a ! in the error-handling logic, so everything broke when #checkPageErrors generated an AwfulError. This actually caused two big issues (and probably more weird behaviour): - non-critical errors (i.e. probations) were being treated as unhandled critical errors and halting the whole request process with a failure - not-logged-in errors (which you get when your cookie expires) were being treated as handled errors and basically acting as a successful request even though they'd failed So I fixed that logic - now critical errors fail unless handled, and non-critical ones are ignored. I've also fixed up a couple of request classes that weren't ignoring non-critical errors (so they didn't work while on probation) I've also fixed the cookie expiry logic in CookieController#restoreLoginCookies which is also used as a check for if the user is logged in - the validity check wasn't looking to see if the cookie had expired, so it could say you were logged in but actually end up with no cookies in use (because they'd all expired). This wasn't actually the problem - loading a page without any cookies should return a page with the "not logged in" message that generates an AwfulError and kicks you to the login dialog, but the above issue with critical errors being swallowed was stopping that from happening. But this logic is more correct anyway. Fixes #684, fixes #596 --- .../java/com/ferg/awfulapp/forums/UpdateTask.kt | 2 ++ .../ferg/awfulapp/network/CookieController.java | 15 +++++++-------- .../java/com/ferg/awfulapp/task/AwfulRequest.kt | 3 ++- .../ferg/awfulapp/task/LepersColonyRequest.kt | 2 -- .../java/com/ferg/awfulapp/util/AwfulError.java | 16 ++++++++-------- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt b/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt index 2ca2a35bd..ae45c506e 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt @@ -236,6 +236,8 @@ internal abstract class UpdateTask(protected val context: Context, private val t override fun handleError(error: AwfulError, doc: Document): Boolean { + if (!error.isCritical) return true + onRequestFailed(error) finishTask(false) return true diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/network/CookieController.java b/Awful.apk/src/main/java/com/ferg/awfulapp/network/CookieController.java index eeb584a7f..0fdaf7f0c 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/network/CookieController.java +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/network/CookieController.java @@ -72,12 +72,15 @@ public static synchronized boolean restoreLoginCookies(Context ctx) { long expiry = prefs.getLong(Constants.COOKIE_PREF_EXPIRY_DATE, -1); int cookieVersion = prefs.getInt(Constants.COOKIE_PREF_VERSION, 0); - if (useridCookieValue == null || passwordCookieValue == null || expiry == -1) { + long maxAge = expiry - System.currentTimeMillis(); + boolean cookieExpired = maxAge <= 0; + // verify the cookie is valid - if not, we need to clear the cookie and return a failure + if (useridCookieValue == null || passwordCookieValue == null || cookieExpired) { if (Constants.DEBUG) { Timber.w("Unable to restore cookies! Reasons:\n" + (useridCookieValue == null ? "USER_ID is NULL\n" : "") + (passwordCookieValue == null ? "PASSWORD is NULL\n" : "") + - (expiry == -1 ? "EXPIRY is -1" : "")); + (cookieExpired ? "cookie has expired, max age = " + maxAge : "")); } cookie = ""; @@ -90,8 +93,6 @@ public static synchronized boolean restoreLoginCookies(Context ctx) { Constants.COOKIE_NAME_SESSIONID, sessionidCookieValue, Constants.COOKIE_NAME_SESSIONHASH, sessionhashCookieValue); - Date expiryDate = new Date(expiry); - Date now = new Date(); HttpCookie[] allCookies = { new HttpCookie(Constants.COOKIE_NAME_USERID, useridCookieValue), @@ -100,8 +101,6 @@ public static synchronized boolean restoreLoginCookies(Context ctx) { new HttpCookie(Constants.COOKIE_NAME_SESSIONHASH, sessionhashCookieValue) }; - long maxAge = expiryDate.getTime() - now.getTime(); - Timber.e("now.compareTo(expiryDate):%s", maxAge); for (HttpCookie tempCookie : allCookies) { tempCookie.setVersion(cookieVersion); @@ -113,8 +112,8 @@ public static synchronized boolean restoreLoginCookies(Context ctx) { } if (Constants.DEBUG) { - Timber.w("Cookies restored from prefs"); - Timber.w("Cookie dump: %s", TextUtils.join("\n", cookieManager.getCookieStore().getCookies())); + Timber.i("Cookies restored from prefs"); + Timber.i("Cookie dump: %s", TextUtils.join("\n", cookieManager.getCookieStore().getCookies())); } return true; diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt b/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt index 60bd760f9..7975e658f 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt @@ -168,6 +168,7 @@ abstract class AwfulRequest(protected val context: Context, private val baseU * By default this swallows non-critical errors, and returns false for everything else. If you need * different behaviour for some reason, override this! */ + // TODO: decide whether probations (or any other non-critical thing) should even be an AwfulError because this and every override basically has to ignore them protected open fun handleError(error: AwfulError, doc: Document): Boolean = !error.isCritical /** @@ -248,7 +249,7 @@ abstract class AwfulRequest(protected val context: Context, private val baseU val doc = parseAsHtml(response) updateProgress(50) val error = AwfulError.checkPageErrors(doc, preferences) - if (error != null && handleError(error, doc)) { + if (error != null && !handleError(error, doc)) { throw error } diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/task/LepersColonyRequest.kt b/Awful.apk/src/main/java/com/ferg/awfulapp/task/LepersColonyRequest.kt index 482301b17..5d1e88bea 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/task/LepersColonyRequest.kt +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/task/LepersColonyRequest.kt @@ -6,7 +6,6 @@ import com.ferg.awfulapp.constants.Constants.* import com.ferg.awfulapp.task.LepersColonyRequest.LepersColonyPage import com.ferg.awfulapp.users.LepersColonyFragment.Companion.FIRST_PAGE import com.ferg.awfulapp.users.Punishment -import com.ferg.awfulapp.util.AwfulError import org.jsoup.nodes.Document /** @@ -56,7 +55,6 @@ class LepersColonyRequest(context: Context, val page: Int = 1, val userId: Strin return LepersColonyPage(punishments, thisPage, lastPage, userId) } - override fun handleError(error: AwfulError, doc: Document) = false /** * Represents the contents and metadata for a page from the Leper's Colony diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java b/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java index d4e5747c8..2703d6119 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java @@ -32,7 +32,6 @@ * requests we make can focus on looking for specific problems. */ public class AwfulError extends VolleyError { - private static final String TAG = "AwfulError"; public static final int ERROR_LOGGED_OUT = 0x00000001; public static final int ERROR_FORUM_CLOSED = 0x00000002; @@ -40,6 +39,8 @@ public class AwfulError extends VolleyError { public static final int ERROR_GENERIC_FAILURE = 0x00000008; public static final int ERROR_ACCESS_DENIED = 0x00000010; + private static final Pattern PROBATION_MESSAGE_REGEX = Pattern.compile("(.*)until\\s(([\\s\\w:,])+).\\sYou(.*)"); + private final int errorCode; @Nullable private final String errorMessage; @@ -60,7 +61,6 @@ public AwfulError(@Nullable String message) { public AwfulError(int code, @Nullable String message) { errorCode = code; errorMessage = message; - Timber.e("Error: " + code + " - " + getMessage()); } /** @@ -157,7 +157,7 @@ public static AwfulError checkPageErrors(Document page, AwfulPreferences prefs) } } - // you're probed! + // handle probation status by looking for the probation message (or lack of it) Element probation = page.getElementById("probation_warn"); if (probation == null) { // clear any probation @@ -171,9 +171,8 @@ public static AwfulError checkPageErrors(Document page, AwfulPreferences prefs) } // try to parse the probation date - default to 1 day in case we can't parse it (not too scary) - Long probTimestamp = System.currentTimeMillis() + TimeUnit.DAYS.toMillis(1); - Pattern p = Pattern.compile("(.*)until\\s(([\\s\\w:,])+).\\sYou(.*)"); - Matcher m = p.matcher(probation.text()); + long probTimestamp = System.currentTimeMillis() + TimeUnit.DAYS.toMillis(1); + Matcher m = PROBATION_MESSAGE_REGEX.matcher(probation.text()); if (m.find()) { String date = m.group(2); //for example January 11, 2013 10:35 AM CST @@ -182,13 +181,14 @@ public static AwfulError checkPageErrors(Document page, AwfulPreferences prefs) //TODO this might have timezone issues? probTimestamp = probationFormat.parse(date).getTime(); } catch (ParseException e) { - Timber.tag(TAG).w(e, "checkPageErrors: couldn't parse probation date text: %s", date); + Timber.w(e, "checkPageErrors: couldn't parse probation date text: %s", date); } } else { - Timber.tag(TAG).w("checkPageErrors: couldn't find expected probation date text!\nFull text: %s", probation.text()); + Timber.w("checkPageErrors: couldn't find expected probation date text!\nFull text: %s", probation.text()); } prefs.setPreference(Keys.PROBATION_TIME, probTimestamp); + // TODO: decide if this should really be an AwfulError, because a whole lot of code exists to check for this just to ignore it when handling errors (see AwfulRequest#handleError) return new AwfulError(ERROR_PROBATION); } return null; From c03df71ea82238ab7dacc006c75ea4bc99feb70a Mon Sep 17 00:00:00 2001 From: baka kaba Date: Sat, 13 Apr 2019 00:29:43 +0100 Subject: [PATCH 2/3] Remove non-critical AwfulErrors from error handling We had a situation where our error-handling code (and anything overriding it) had to do a check for non-critical errors and return a "handled" response. Nothing ever cared about doing anything with these (which at this moment are only "you're probated" AwfulErrors), so I've removed all that boilerplate handling code and made the main request handler code do a check, so it only passes critical errors for handling. This means individual requests can't handle non-critical errors in any special way anymore, but we only have one kind and there's no reason to handle it anyway. But if that changes in the future (or we add new and exciting non-critical errors) this might need to be rolled back --- .../com/ferg/awfulapp/forums/UpdateTask.kt | 4 +-- .../com/ferg/awfulapp/task/AwfulRequest.kt | 26 +++++++++---------- .../com/ferg/awfulapp/task/LoginRequest.kt | 4 +-- .../com/ferg/awfulapp/util/AwfulError.java | 1 - 4 files changed, 16 insertions(+), 19 deletions(-) diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt b/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt index ae45c506e..a50717448 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/forums/UpdateTask.kt @@ -235,9 +235,7 @@ internal abstract class UpdateTask(protected val context: Context, private val t } - override fun handleError(error: AwfulError, doc: Document): Boolean { - if (!error.isCritical) return true - + override fun handleCriticalError(error: AwfulError, doc: Document): Boolean { onRequestFailed(error) finishTask(false) return true diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt b/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt index 7975e658f..564aeeba8 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/task/AwfulRequest.kt @@ -47,9 +47,9 @@ import java.io.IOException * do anything with the response (e.g. a fire-and-forget message to the site) you can just set [T] * to a nullable type (Void? makes most sense if there's no meaningful result) and return null here. * - * [handleError] and [customizeProgressListenerError] both have default implementations, but can be + * [handleCriticalError] and [customizeProgressListenerError] both have default implementations, but can be * overridden for special error handling and creating custom notification messages. You probably - * won't want to change [handleError] in most cases, and if you do add any handler code (e.g. if + * won't want to change [handleCriticalError] in most cases, and if you do add any handler code (e.g. if * a network failure requires the app to update some state) you'll probably just want to call the * super method to get the standard error handling when you're done. */ @@ -159,17 +159,17 @@ abstract class AwfulRequest(protected val context: Context, private val baseU /** - * Handler for [error]s thrown by [AwfulError.checkPageErrors] when parsing a response. + * Handler for critical [error]s thrown by [AwfulError.checkPageErrors] when parsing a response. * - * The main response-handler logic calls this when it encounters an [AwfulError], to check whether - * the request implementation will handle it (and processing can proceed to [handleResponse]). - * Returns true if the error was handled. + * The main response-handler logic calls this when it encounters a critical [AwfulError], + * to check whether the request implementation will handle it (and processing can proceed + * to [handleResponse]). Returns true if the error was handled. * - * By default this swallows non-critical errors, and returns false for everything else. If you need - * different behaviour for some reason, override this! + * This is only called for errors that return true for [AwfulError.isCritical], and returns + * false by default. If you want to handle any of these errors, override this and return true + * for those. */ - // TODO: decide whether probations (or any other non-critical thing) should even be an AwfulError because this and every override basically has to ignore them - protected open fun handleError(error: AwfulError, doc: Document): Boolean = !error.isCritical + protected open fun handleCriticalError(error: AwfulError, doc: Document): Boolean = false /** * Customize the error a request delivers in its [ProgressListener.requestEnded] callback. @@ -248,9 +248,9 @@ abstract class AwfulRequest(protected val context: Context, private val baseU try { val doc = parseAsHtml(response) updateProgress(50) - val error = AwfulError.checkPageErrors(doc, preferences) - if (error != null && !handleError(error, doc)) { - throw error + // we only pass critical errors for requests to handle - anything else (i.e. probations) gets swallowed + AwfulError.checkPageErrors(doc, preferences)?.let { error -> + if (error.isCritical && !handleCriticalError(error, doc)) throw error } val result = handleResponseDocument(doc) diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/task/LoginRequest.kt b/Awful.apk/src/main/java/com/ferg/awfulapp/task/LoginRequest.kt index 8fd432aab..59cabeea1 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/task/LoginRequest.kt +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/task/LoginRequest.kt @@ -26,8 +26,8 @@ class LoginRequest(context: Context, private val username: String, password: Str @Throws(AwfulError::class) override fun handleResponse(doc: Document): Boolean = validateLoginState() - override fun handleError(error: AwfulError, doc: Document): Boolean = - error.networkResponse?.isRedirect == true || !error.isCritical + override fun handleCriticalError(error: AwfulError, doc: Document): Boolean = + error.networkResponse?.isRedirect == true private val NetworkResponse.isRedirect get() = this.statusCode == 302 diff --git a/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java b/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java index 2703d6119..44a3e8274 100644 --- a/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java +++ b/Awful.apk/src/main/java/com/ferg/awfulapp/util/AwfulError.java @@ -188,7 +188,6 @@ public static AwfulError checkPageErrors(Document page, AwfulPreferences prefs) } prefs.setPreference(Keys.PROBATION_TIME, probTimestamp); - // TODO: decide if this should really be an AwfulError, because a whole lot of code exists to check for this just to ignore it when handling errors (see AwfulRequest#handleError) return new AwfulError(ERROR_PROBATION); } return null; From fb14e607ae2e8b8de566d2b4352b4ee484368374 Mon Sep 17 00:00:00 2001 From: baka kaba Date: Sat, 13 Apr 2019 02:51:29 +0100 Subject: [PATCH 3/3] Bump to 3.6.5 and update changelog --- Awful.apk/src/main/AndroidManifest.xml | 4 ++-- Awful.apk/src/main/assets/changelog.html | 9 ++++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/Awful.apk/src/main/AndroidManifest.xml b/Awful.apk/src/main/AndroidManifest.xml index d4b142a2e..c1e1573e5 100644 --- a/Awful.apk/src/main/AndroidManifest.xml +++ b/Awful.apk/src/main/AndroidManifest.xml @@ -1,8 +1,8 @@
-

3.6.3

+

3.6.5

+
    +
  • Important fixes for probations and other weird behavior (the app's, not yours...)
    • +
+
+ +
+

3.6.4

  • Fixed broken stuff from the terrible non-https internet badlands
  • WebP is an image format, we knew that, so do our menus