Issue 303 - Don't load element hiding rules

src/org/adblockplus/android/AndroidWebRequest.java

 /*
- * This file is part of Adblock Plus <http://adblockplus.org/>,
- * Copyright (C) 2006-2014 Eyeo GmbH
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 package org.adblockplus.android;
 
 import java.io.BufferedReader;
 import java.io.InputStreamReader;
 import java.net.HttpURLConnection;
 import java.net.URL;
+import java.util.HashSet;
 import java.util.List;
-import java.util.Locale;
 
 import org.adblockplus.libadblockplus.AdblockPlusException;
+import org.adblockplus.libadblockplus.FilterEngine;
 import org.adblockplus.libadblockplus.HeaderEntry;
 import org.adblockplus.libadblockplus.ServerResponse;
 import org.adblockplus.libadblockplus.ServerResponse.NsStatus;
 import org.adblockplus.libadblockplus.WebRequest;
-import org.apache.commons.lang.StringUtils;
 
 import android.util.Log;
 
 public class AndroidWebRequest extends WebRequest
 {
-  public final String TAG = Utils.getTag(WebRequest.class);
+  public final static String TAG = Utils.getTag(WebRequest.class);
 
-  private static boolean mightBeFilterList(URL url)
+  private final HashSet<String> subscriptionURLs = new HashSet<String>();
+
+  private boolean isListedSubscriptionUrl(final URL url)
   {
-    String filename = url.getFile();
+    String toCheck = url.toString();
 
-    if (StringUtils.isEmpty(filename))
+    final int idx = toCheck.indexOf('?');
+    if (idx != -1)
     {
-      return false;
+      toCheck = toCheck.substring(0, idx);
     }
 
-    final int params = filename.indexOf('?');
+    return this.subscriptionURLs.contains(toCheck);
+  }
 
-    if (params != -1)
+  protected void updateSubscriptionURLs(final FilterEngine engine)
+  {
+    for (final org.adblockplus.libadblockplus.Subscription s : engine.fetchAvailableSubscriptions())
     {
-      filename = filename.substring(0, params);

[Felix Dahlke, 2014/11/11 09:15:37]

Why not just use url.getPath() instead of manually cutting off the query string?
According to the docs, url.getFile() is the same as url.getPath() +
url.getQuery().

[René Jeschke, 2014/11/11 11:55:55]

Right, done that.

+      this.subscriptionURLs.add(s.getProperty("url").toString());
     }
-
-    return filename.toLowerCase(Locale.US).trim().endsWith(".txt");

[Felix Dahlke, 2014/11/11 09:15:37]

I think I might have thought of a better way to determine whether the request is
for a filter list: We can go through all active subscriptions and check their
URL. We'd have to ensure that the subscription is first added to the list of
known subscriptions before the request happens on first start.

That'd allow us to give this method name a bit more conviction, e.g.
"isSubscriptionUrl" or something.

[René Jeschke, 2014/11/11 11:55:55]

I think that this is a bit much of an overhead for a hack. The Android app does
not load anything else than filter lists or update JSONs, so a check for .txt
should really be Ok for now.

[Felix Dahlke, 2014/11/11 16:18:30]

It currently doesn't, but I don't think hard coding such assumptions here is a
good idea if we can easily avoid it. We might make requests for *.txt files that
aren't filter lists. And, more likely, we might make requests for filter lists
that don't match *.txt. I believe we should try to be more robust here, if we
can.

Going over all subscription URLs here should be really negligible overhead-wise
- we're only making about one request per day per filter list, slowing that one
down by maybe 10ms shouldn't be noticeable at all. On the other hand, we could
be 100% sure that it's a filter list request, no matter what.

+    this.subscriptionURLs.add(engine.getPref("subscriptions_exceptionsurl").toString());
   }
 
   @Override
   public ServerResponse httpGET(final String urlStr, final List<HeaderEntry> headers)
   {
     try
     {
       final URL url = new URL(urlStr);
-      Log.d(this.TAG, "Downloading from: " + url);
-
-      final boolean mightBeFilterList = mightBeFilterList(url);
+      Log.d(TAG, "Downloading from: " + url);
 
       final HttpURLConnection connection = (HttpURLConnection) url.openConnection();
       connection.setRequestMethod("GET");
       connection.connect();
 
       final ServerResponse response = new ServerResponse();
       response.setResponseStatus(connection.getResponseCode());
 
       if (response.getResponseStatus() == 200)
       {
         final BufferedReader reader = new BufferedReader(new InputStreamReader(connection.getInputStream(), "UTF-8"));
         final StringBuilder sb = new StringBuilder();
 
-        String line;
-        while ((line = reader.readLine()) != null)

[Felix Dahlke, 2014/11/11 09:15:37]

Using BufferedReader instead of System.arraycopy seems fine for all requests we
make here, but I think we shouldn't read anything but filter lists line-by-line.

[René Jeschke, 2014/11/11 11:55:55]

Ok, we'll read the rest char-by-char.

+        if (isListedSubscriptionUrl(url))
         {
-          // We're only appending non-element-hiding filters here.
-          // See: https://issues.adblockplus.org/ticket/303
-          if (!mightBeFilterList || line.indexOf('#') == -1)
+          Log.d(TAG, "Removing element hiding rules from: '" + url + "'");
+
+          String line;
+          while ((line = reader.readLine()) != null)
           {
-            sb.append(line);
-            sb.append('\n');
+            // We're only appending non-element-hiding filters here.
+            //
+            // See:
+            //      https://issues.adblockplus.org/ticket/303
+            //
+            // Follow-up issue for removing this hack:
+            //      https://issues.adblockplus.org/ticket/1541
+            //
+            if (line.indexOf('#') == -1)
+            {
+              sb.append(line);
+              sb.append('\n');
+            }
           }
         }
+        else
+        {
+          int character;
+
+          while ((character = reader.read()) != -1)
+          {
+            sb.append((char) character);
+          }
+        }
+
         connection.disconnect();
 
         response.setStatus(NsStatus.OK);
         response.setResponse(sb.toString());
       }
       else
       {
         response.setStatus(NsStatus.ERROR_FAILURE);
       }
       return response;
     }
     catch (final Throwable t)
     {
       throw new AdblockPlusException("WebRequest failed", t);
     }
   }
 }