Bug 1241810 - Review follow-up: Avoid logging potential personal information. r=me draft
authorSebastian Kaspari <s.kaspari@gmail.com>
Thu, 03 Mar 2016 13:28:08 +0100
changeset 343944 c5f93e7093027a5ce2c1f8d45c6a351a44a70a90
parent 343943 9e87f1a3ae1dc6ebcdeb6ceab5948901191d0828
child 343945 d07e323506184a528c40d9f6fa25311a86dbb45b
push id13713
push users.kaspari@gmail.com
push dateWed, 23 Mar 2016 15:10:41 +0000
reviewersme
bugs1241810
milestone48.0a1
Bug 1241810 - Review follow-up: Avoid logging potential personal information. r=me MozReview-Commit-ID: BsGgce12yWd
mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java
mobile/android/base/java/org/mozilla/gecko/feeds/action/BaseAction.java
mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java
mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java
mobile/android/base/java/org/mozilla/gecko/feeds/action/SetupAction.java
mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java
mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java
--- a/mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java
+++ b/mobile/android/base/java/org/mozilla/gecko/feeds/FeedService.java
@@ -98,16 +98,18 @@ public class FeedService extends IntentS
                 Log.d(LOGTAG, "Not connected to a network or network is metered. Skipping.");
                 return;
             }
 
             action.perform(browserDB, intent);
         } finally {
             FeedAlarmReceiver.completeWakefulIntent(intent);
         }
+
+        Log.d(LOGTAG, "Done.");
     }
 
     @Nullable
     private BaseAction createActionForIntent(Intent intent) {
         final Context context = getApplicationContext();
 
         switch (intent.getAction()) {
             case ACTION_SETUP:
--- a/mobile/android/base/java/org/mozilla/gecko/feeds/action/BaseAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/feeds/action/BaseAction.java
@@ -1,33 +1,58 @@
 /* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.feeds.action;
 
 import android.content.Intent;
+import android.util.Log;
 
 import org.mozilla.gecko.db.BrowserDB;
 
 /**
  * Interface for actions run by FeedService.
  */
-public interface BaseAction {
+public abstract class BaseAction {
+    public static final boolean DEBUG_LOG = false;
+
     /**
      * Perform this action.
-     * 
+     *
      * @param browserDB database instance to perform the action.
      * @param intent used to start the service.
      */
-    void perform(BrowserDB browserDB, Intent intent);
+    public abstract void perform(BrowserDB browserDB, Intent intent);
 
     /**
      * Does this action require an active network connection?
      */
-    boolean requiresNetwork();
+    public abstract boolean requiresNetwork();
 
-	/**
+    /**
      * Should this action only run if the preference is enabled?
      */
-    boolean requiresPreferenceEnabled();
+    public abstract boolean requiresPreferenceEnabled();
+
+    /**
+     * This method will swallow all log messages to avoid logging potential personal information.
+     *
+     * For debugging purposes set {@code DEBUG_LOG} to true.
+     */
+    public void log(String message) {
+        if (DEBUG_LOG) {
+            Log.d("Gecko" + getClass().getSimpleName(), message);
+        }
+    }
+
+    /**
+     * This method will swallow all log messages to avoid logging potential personal information.
+     *
+     * For debugging purposes set {@code DEBUG_LOG} to true.
+     */
+    public void log(String message, Throwable throwable) {
+        if (DEBUG_LOG) {
+            Log.d("Gecko" + getClass().getSimpleName(), message, throwable);
+        }
+    }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/feeds/action/CheckAction.java
@@ -12,17 +12,16 @@ import android.content.ContentResolver;
 import android.content.Context;
 import android.content.Intent;
 import android.database.Cursor;
 import android.net.Uri;
 import android.support.v4.app.NotificationCompat;
 import android.support.v4.app.NotificationManagerCompat;
 import android.support.v4.content.ContextCompat;
 import android.text.format.DateFormat;
-import android.util.Log;
 
 import org.json.JSONException;
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.BrowserApp;
 import org.mozilla.gecko.GeckoApp;
 import org.mozilla.gecko.R;
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.TelemetryContract;
@@ -36,17 +35,17 @@ import org.mozilla.gecko.util.StringUtil
 
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 
 /**
  * CheckAction: Check if feeds we subscribed to have new content available.
  */
-public class CheckAction implements BaseAction {
+public class CheckAction extends BaseAction {
     /**
      * This extra will be added to Intents fired by the notification.
      */
     public static final String EXTRA_CONTENT_NOTIFICATION = "content-notification";
 
     private static final String LOGTAG = "FeedCheckAction";
 
     private Context context;
@@ -56,17 +55,17 @@ public class CheckAction implements Base
     }
 
     @Override
     public void perform(BrowserDB browserDB, Intent intent) {
         final UrlAnnotations urlAnnotations = browserDB.getUrlAnnotations();
         final ContentResolver resolver = context.getContentResolver();
         final List<Feed> updatedFeeds = new ArrayList<>();
 
-        Log.d(LOGTAG, "Checking feeds for updates..");
+        log("Checking feeds for updates..");
 
         Cursor cursor = urlAnnotations.getFeedSubscriptions(resolver);
         if (cursor == null) {
             return;
         }
 
         try {
             while (cursor.moveToNext()) {
@@ -75,34 +74,34 @@ public class CheckAction implements Base
                 FeedFetcher.FeedResponse response = checkFeedForUpdates(subscription);
                 if (response != null) {
                     updatedFeeds.add(response.feed);
 
                     urlAnnotations.updateFeedSubscription(resolver, subscription);
                 }
             }
         } catch (JSONException e) {
-            Log.w(LOGTAG, "Could not deserialize subscription", e);
+            log("Could not deserialize subscription", e);
         } finally {
             cursor.close();
         }
 
         notify(updatedFeeds);
     }
 
     private FeedFetcher.FeedResponse checkFeedForUpdates(FeedSubscription subscription) {
-        Log.i(LOGTAG, "Checking feed: " + subscription.getFeedTitle());
+        log("Checking feed: " + subscription.getFeedTitle());
 
         FeedFetcher.FeedResponse response = fetchFeed(subscription);
         if (response == null) {
             return null;
         }
 
-        if (subscription.isNewer(response)) {
-            Log.d(LOGTAG, "* Feed has changed. New item: " + response.feed.getLastItem().getTitle());
+        if (subscription.hasBeenUpdated(response)) {
+            log("* Feed has changed. New item: " + response.feed.getLastItem().getTitle());
 
             subscription.update(response);
 
             return response;
 
         }
 
         return null;
@@ -162,17 +161,17 @@ public class CheckAction implements Base
             inboxStyle.addLine(StringUtils.stripScheme(url, StringUtils.UrlFlags.STRIP_HTTPS));
             urls.add(url);
         }
         inboxStyle.setSummaryText(context.getString(R.string.content_notification_summary));
 
         Intent intent = new Intent(context, BrowserApp.class);
         intent.setAction(BrowserApp.ACTION_VIEW_MULTIPLE);
         intent.putStringArrayListExtra("urls", urls);
-        intent.putExtra(EXTRA_CONTENT_NOTIFICATION, true);
+	    intent.putExtra(EXTRA_CONTENT_NOTIFICATION, true);
 
         PendingIntent pendingIntent = PendingIntent.getActivity(context, 0, intent, 0);
 
         Notification notification = new NotificationCompat.Builder(context)
                 .setSmallIcon(R.drawable.ic_status_logo)
                 .setContentTitle(context.getString(R.string.content_notification_title_plural, feeds.size()))
                 .setContentText(context.getString(R.string.content_notification_summary))
                 .setStyle(inboxStyle)
--- a/mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/feeds/action/EnrollAction.java
@@ -5,49 +5,47 @@
 
 package org.mozilla.gecko.feeds.action;
 
 import android.content.ContentResolver;
 import android.content.Context;
 import android.content.Intent;
 import android.database.Cursor;
 import android.text.TextUtils;
-import android.util.Log;
 
-import org.mozilla.gecko.GeckoProfile;
 import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.db.UrlAnnotations;
 import org.mozilla.gecko.feeds.FeedService;
 import org.mozilla.gecko.feeds.knownsites.KnownSiteBlogger;
 import org.mozilla.gecko.feeds.knownsites.KnownSite;
 import org.mozilla.gecko.feeds.knownsites.KnownSiteMedium;
 import org.mozilla.gecko.feeds.knownsites.KnownSiteWordpress;
 
 /**
  * EnrollAction: Search for bookmarks of known sites we can subscribe to.
  */
-public class EnrollAction implements BaseAction {
+public class EnrollAction extends BaseAction {
     private static final String LOGTAG = "FeedEnrollAction";
 
     private static final KnownSite[] knownSites = {
         new KnownSiteMedium(),
         new KnownSiteBlogger(),
         new KnownSiteWordpress(),
     };
 
     private Context context;
 
     public EnrollAction(Context context) {
         this.context = context;
     }
 
     @Override
     public void perform(BrowserDB db, Intent intent) {
-        Log.i(LOGTAG, "Searching for bookmarks to enroll in updates");
+        log("Searching for bookmarks to enroll in updates");
 
         final ContentResolver contentResolver = context.getContentResolver();
 
         for (KnownSite knownSite : knownSites) {
             searchFor(db, contentResolver, knownSite);
         }
     }
 
@@ -61,34 +59,33 @@ public class EnrollAction implements Bas
         return true;
     }
 
     private void searchFor(BrowserDB db, ContentResolver contentResolver, KnownSite knownSite) {
         final UrlAnnotations urlAnnotations = db.getUrlAnnotations();
 
         final Cursor cursor = db.getBookmarksForPartialUrl(contentResolver, knownSite.getURLSearchString());
         if (cursor == null) {
-            Log.d(LOGTAG, "Nothing found (" + knownSite.getClass().getSimpleName() + ")");
+            log("Nothing found (" + knownSite.getClass().getSimpleName() + ")");
             return;
         }
 
         try {
-            Log.d(LOGTAG, "Found " + cursor.getCount() + " websites");
+            log("Found " + cursor.getCount() + " websites");
 
             while (cursor.moveToNext()) {
 
                 final String url = cursor.getString(cursor.getColumnIndex(BrowserContract.Bookmarks.URL));
 
-                Log.d(LOGTAG, " URL: " + url);
+                log(" URL: " + url);
 
                 String feedUrl = knownSite.getFeedFromURL(url);
                 if (TextUtils.isEmpty(feedUrl)) {
-
-
-                    Log.w(LOGTAG, "Could not determine feed for URL: " + url);
+                    log("Could not determine feed for URL: " + url);
+                    return;
                 }
 
                 if (!urlAnnotations.hasFeedUrlForWebsite(contentResolver, url)) {
                     urlAnnotations.insertFeedUrl(contentResolver, url, feedUrl);
                 }
 
                 if (!urlAnnotations.hasFeedSubscription(contentResolver, feedUrl)) {
                     FeedService.subscribe(context, feedUrl);
--- a/mobile/android/base/java/org/mozilla/gecko/feeds/action/SetupAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/feeds/action/SetupAction.java
@@ -5,26 +5,25 @@
 
 package org.mozilla.gecko.feeds.action;
 
 import android.app.AlarmManager;
 import android.app.PendingIntent;
 import android.content.Context;
 import android.content.Intent;
 import android.os.SystemClock;
-import android.util.Log;
 
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.feeds.FeedAlarmReceiver;
 import org.mozilla.gecko.feeds.FeedService;
 
 /**
  * SetupAction: Set up alarms to run various actions every now and then.
  */
-public class SetupAction implements BaseAction {
+public class SetupAction extends BaseAction {
     private static final String LOGTAG = "FeedSetupAction";
 
     private Context context;
 
     public SetupAction(Context context) {
         this.context = context;
     }
 
@@ -51,17 +50,17 @@ public class SetupAction implements Base
         alarmManager.cancel(withdrawIntent);
 
         final PendingIntent enrollIntent = getEnrollPendingIntent();
         alarmManager.cancel(enrollIntent);
 
         final PendingIntent checkIntent = getCheckPendingIntent();
         alarmManager.cancel(checkIntent);
 
-        Log.d(LOGTAG, "Cancelled previous alarms");
+        log("Cancelled previous alarms");
     }
 
     private void scheduleAlarms(AlarmManager alarmManager) {
         alarmManager.setInexactRepeating(
                 AlarmManager.ELAPSED_REALTIME,
                 SystemClock.elapsedRealtime() + AlarmManager.INTERVAL_FIFTEEN_MINUTES,
                 AlarmManager.INTERVAL_DAY,
                 getWithdrawPendingIntent());
@@ -75,17 +74,17 @@ public class SetupAction implements Base
 
         alarmManager.setInexactRepeating(
                 AlarmManager.ELAPSED_REALTIME,
                 SystemClock.elapsedRealtime() + AlarmManager.INTERVAL_HOUR,
                 AlarmManager.INTERVAL_HALF_DAY,
                 getCheckPendingIntent()
         );
 
-        Log.d(LOGTAG, "Scheduled alarms");
+        log("Scheduled alarms");
     }
 
     private PendingIntent getWithdrawPendingIntent() {
         Intent intent = new Intent(context, FeedAlarmReceiver.class);
         intent.setAction(FeedService.ACTION_WITHDRAW);
         return PendingIntent.getBroadcast(context, 0, intent, 0);
     }
 
--- a/mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/feeds/action/SubscribeAction.java
@@ -3,29 +3,28 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.feeds.action;
 
 import android.content.Context;
 import android.content.Intent;
 import android.os.Bundle;
-import android.util.Log;
 
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.TelemetryContract;
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.db.UrlAnnotations;
 import org.mozilla.gecko.feeds.FeedFetcher;
 import org.mozilla.gecko.feeds.subscriptions.FeedSubscription;
 
 /**
  * SubscribeAction: Try to fetch a feed and create a subscription if successful.
  */
-public class SubscribeAction implements BaseAction {
+public class SubscribeAction extends BaseAction {
     private static final String LOGTAG = "FeedSubscribeAction";
 
     public static final String EXTRA_FEED_URL = "feed_url";
 
     private Context context;
 
     public SubscribeAction(Context context) {
         this.context = context;
@@ -34,21 +33,21 @@ public class SubscribeAction implements 
     @Override
     public void perform(BrowserDB browserDB, Intent intent) {
         final UrlAnnotations urlAnnotations = browserDB.getUrlAnnotations();
 
         final Bundle extras = intent.getExtras();
         final String feedUrl = extras.getString(EXTRA_FEED_URL);
 
         if (urlAnnotations.hasFeedSubscription(context.getContentResolver(), feedUrl)) {
-            Log.d(LOGTAG, "Already subscribed to " + feedUrl + ". Skipping.");
+            log("Already subscribed to " + feedUrl + ". Skipping.");
             return;
         }
 
-        Log.d(LOGTAG, "Subscribing to feed: " + feedUrl);
+        log("Subscribing to feed: " + feedUrl);
 
         subscribe(urlAnnotations, feedUrl);
     }
 
     @Override
     public boolean requiresNetwork() {
         return true;
     }
@@ -56,22 +55,22 @@ public class SubscribeAction implements 
     @Override
     public boolean requiresPreferenceEnabled() {
         return true;
     }
 
     private void subscribe(UrlAnnotations urlAnnotations, String feedUrl) {
         FeedFetcher.FeedResponse response = FeedFetcher.fetchAndParseFeed(feedUrl);
         if (response == null) {
-            Log.w(LOGTAG, String.format("Could not fetch feed (%s). Not subscribing for now.", feedUrl));
+            log(String.format("Could not fetch feed (%s). Not subscribing for now.", feedUrl));
             return;
         }
 
-        Log.d(LOGTAG, "Subscribing to feed: " + response.feed.getTitle());
-        Log.d(LOGTAG, "          Last item: " + response.feed.getLastItem().getTitle());
+        log("Subscribing to feed: " + response.feed.getTitle());
+        log("          Last item: " + response.feed.getLastItem().getTitle());
 
         final FeedSubscription subscription = FeedSubscription.create(feedUrl, response);
 
         urlAnnotations.insertFeedSubscription(context.getContentResolver(), subscription);
 
         Telemetry.sendUIEvent(TelemetryContract.Event.SAVE, TelemetryContract.Method.SERVICE, "content_update");
     }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java
+++ b/mobile/android/base/java/org/mozilla/gecko/feeds/action/WithdrawAction.java
@@ -4,39 +4,38 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.feeds.action;
 
 import android.content.ContentResolver;
 import android.content.Context;
 import android.content.Intent;
 import android.database.Cursor;
-import android.util.Log;
 
 import org.json.JSONException;
 import org.mozilla.gecko.db.BrowserContract;
 import org.mozilla.gecko.db.BrowserDB;
 import org.mozilla.gecko.db.UrlAnnotations;
 import org.mozilla.gecko.feeds.subscriptions.FeedSubscription;
 
 /**
  * WithdrawAction: Look for feeds to unsubscribe from.
  */
-public class WithdrawAction implements BaseAction {
+public class WithdrawAction extends BaseAction {
     private static final String LOGTAG = "FeedWithdrawAction";
 
     private Context context;
 
     public WithdrawAction(Context context) {
         this.context = context;
     }
 
     @Override
     public void perform(BrowserDB browserDB, Intent intent) {
-        Log.d(LOGTAG, "Searching for subscriptions to remove..");
+        log("Searching for subscriptions to remove..");
 
         final UrlAnnotations urlAnnotations = browserDB.getUrlAnnotations();
         final ContentResolver resolver = context.getContentResolver();
 
         removeFeedsOfUnknownUrls(browserDB, urlAnnotations, resolver);
         removeSubscriptionsOfRemovedFeeds(urlAnnotations, resolver);
     }
 
@@ -50,17 +49,17 @@ public class WithdrawAction implements B
             return;
         }
 
         try {
             while (cursor.moveToNext()) {
                 final String url = cursor.getString(cursor.getColumnIndex(BrowserContract.UrlAnnotations.URL));
 
                 if (!browserDB.isBookmark(resolver, url)) {
-                    Log.d(LOGTAG, "Removing feed for unknown URL: " + url);
+                    log("Removing feed for unknown URL: " + url);
 
                     urlAnnotations.deleteFeedUrl(resolver, url);
                 }
             }
         } finally {
             cursor.close();
         }
     }
@@ -74,23 +73,23 @@ public class WithdrawAction implements B
             return;
         }
 
         try {
             while (cursor.moveToNext()) {
                 final FeedSubscription subscription = FeedSubscription.fromCursor(cursor);
 
                 if (!urlAnnotations.hasWebsiteForFeedUrl(resolver, subscription.getFeedUrl())) {
-                    Log.d(LOGTAG, "Removing subscription for feed: " + subscription.getFeedUrl());
+                    log("Removing subscription for feed: " + subscription.getFeedUrl());
 
                     urlAnnotations.deleteFeedSubscription(resolver, subscription);
                 }
             }
         } catch (JSONException e) {
-            Log.w(LOGTAG, "Could not deserialize subscription", e);
+            log("Could not deserialize subscription", e);
         } finally {
             cursor.close();
         }
     }
 
     @Override
     public boolean requiresNetwork() {
         return false;