Issue 1393 - Block element dialog suggests decoded URLs

include.postload.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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
@@ skipping 261 matching lines @@
   var elt = currentElement;
   var url = null;
   if (currentElement.className && currentElement.className == "__adblockplus__overlay")
   {
     elt = currentElement.prisoner;
     url = currentElement.prisonerURL;
   }
   else if (elt.src)
     url = elt.src;
 
-  // Only normalize when the element contains a URL (issue 328.)
-  // The URL is not always normalized, so do it here
-  if (url)
-    url = normalizeURL(relativeToAbsoluteUrl(url));
-
   // Construct filters. The popup will retrieve these.
   // Only one ID
   var elementId = elt.id ? elt.id.split(' ').join('') : null;
   // Can have multiple classes, and there might be extraneous whitespace
   var elementClasses = null;
   if (elt.className)
     elementClasses = elt.className.replace(/\s+/g, ' ').replace(/^\s+|\s+$/g, '').split(' ');
 
   clickHideFilters = new Array();
   selectorList = new Array();
@@ skipping 45 matching lines @@
     // No data attribute, look in PARAM child tags for a URL for the swf file
     var params = elt.querySelectorAll("param[name=\"movie\"]");
     // This OBJECT could contain an EMBED we already nuked, in which case there's no URL
     if(params[0])
       url = params[0].getAttribute("value");
     else {
       params = elt.querySelectorAll("param[name=\"src\"]");
       if(params[0])
         url = params[0].getAttribute("value");
     }
+
+    if (url)
+      url = normalizeURL(relativeToAbsoluteUrl(url));

[Sebastian Noack, 2014/09/22 13:27:49]

I just realized that Chrome preserves slashes in URLs, while normalizeURL()
normalizes them (e.g. foo//bar -> foo/bar).

So this patch will introduce an inconsistent behavior, since URLs retrieved via
.src and .href aren't passed to normalizeURL() anymore.

[Sebastian Noack, 2014/09/22 13:37:36]

Also unencoded special characters aren't encoded by our normalizeURL() function.
So the original issue will still persist for <object> and <embed> elements.

Maybe we should just implement normalizeURL() like following. This reduces
complexity, is faster, and makes sure that the URL is normalized the same way
the browser does.

function normalizeURL(url)
{
    var frame = document.createElement("iframe");
    frame.src = url;
    return frame.src;
}

[Thomas Greiner, 2014/09/22 15:08:28]

The question is whether we need to have a well-formed URL because what we need
is the URL the way it is requested by the browser. So if the browser requests
"http://example.com//test.jpg" then we shouldn't normalize it to
"http://example.com/test.jpg" since the filter wouldn't match.

On 2014/09/22 13:37:36, Sebastian Noack wrote:
True, maybe that's a necessary hack but I'd rather use <a> tags for that since
they're generally more light-weight.

[Sebastian Noack, 2014/09/22 15:25:16]

I don't say that the we should strip repeated slashes. I just say that we
shouldn't introduce inconsistent behavior. And yes, preferable the behavior
should be the default behavior of the browser.
I think this is less a hack than rolling our own implementation that that mimics
the way the browser normalizes URLs.
Probably that doesn't make any difference. Note that frames are neither loaded
before the element is inserted into the document.

[Sebastian Noack, 2014/09/22 15:50:59]

Actually using an <iframe> element is notable faster (for some reason). Also
caching the element gives a significant boost: http://jsperf.com/normailzeurl

[Wladimir Palant, 2014/09/22 17:39:21]

That should be because Chrome does some caching of its own for frames, please
compare: http://jsperf.com/normalizeurl

So I'd be in favor of using the <a> element, if only to avoid the impression
that something is being downloaded there.

Note that even then we won't get it entirely correct: in case of the <object>
and <applet> tags the URL resolution is being performed by the plugin, and it
might use a different way to resolve URLs from what the browser does (I've hit
that issue with Java before). Plus, there can be different base URLs in a
document (see Node.baseURI). Still, this approach is a lot better than the
current hack.

[Sebastian Noack, 2014/09/23 09:40:34]

Very interesting. So I agree with your point for using an <a> element here.
Though <iframe> elements still seem to be faster when cached, while caching the
<a> element doesn't make much a difference, on Chrome:
http://jsperf.com/normalizeurl/2

However, caching the <a> element might still be worth for Safari, also
considering that we have to resolve the URL for every request there. 
That is true, but there is probably no way to resolve the URL correctly for
every possible plugin. And reusing the built-in URL normalization of the
browser, instead rolling our own, reduces at least complexity and is also
faster.

[Sebastian Noack, 2014/09/25 07:53:18]

LGTM. I created a separate issue, for using an <a> element, when explicitly
resolving URLs: https://issues.adblockplus.org/ticket/1441

   } else if(!url) {
-    url = elt.getAttribute("src") || elt.getAttribute("href");
+    url = elt.src || elt.href;
   }
   return url;
 }
 
 // This function Copyright (c) 2008 Jeni Tennison, from jquery.uri.js
 // and licensed under the MIT license. See jquery-*.min.js for details.
 function removeDotSegments(u) {
   var r = '', m = [];
   if (/\./.test(u)) {
     while (u !== undefined && u !== '') {
@@ skipping 16 matching lines @@
     }
     return r;
   } else {
     return u;
   }
 }
 
 // Does some degree of URL normalization
 function normalizeURL(url)
 {
-  url = encodeURI(url);

[Wladimir Palant, 2014/09/18 18:53:02]

What if the URL we get is already encoded? This function will turn "%20" into
"%2520".

[Sebastian Noack, 2014/09/19 10:13:32]

Thomas, can you please explain / figure out why this is necessary anyway? I just
realized that the URL comes form Element.src and therefore should always already
be quoted.

[Thomas Greiner, 2014/09/19 10:47:13]

Now I see what you mean. Element.src does give us the encoded URL but we're
using Element.getAttribute("src") in the getElementURL() function instead which
gives us the decoded URL.

On 2014/09/19 10:13:32, Sebastian Noack wrote:

[Sebastian Noack, 2014/09/19 11:14:50]

Yes, .src always returns a quoted URL no matter how the URL was specified in the
document.

So the correct solution would be just using .src instead .getAttribute("src")
here, I guess.

[Wladimir Palant, 2014/09/21 18:37:42]

The other advantage here is that element.src returns the full URL and we don't
have to apply our error-prone normalization to it. So maybe we only need to keep
that normalization for objects/applets - no way to get the full URL from the
browser there IMHO.

[Thomas Greiner, 2014/09/22 09:37:33]

I didn't find any reason against applying it to objects/applets-only so I moved
it accordingly. However, I'm not sure what issue 328 is about. Do you happen to
know where I can find it?

On 2014/09/19 11:14:50, Sebastian Noack wrote:
I observed the same behavior with .href and .getAttribute("href") and therefore
changed that as well.

   var components = url.match(/(.+:\/\/.+?)\/(.*)/);
   if(!components)
     return url;
   var newPath = removeDotSegments(components[2]);
   if(newPath.length == 0)
     return components[1];
   if(newPath[0] != '/')
     newPath = '/' + newPath;
   return components[1] + newPath;
 }
@@ skipping 159 matching lines @@
 
           clickHide_deactivate();
         }
         break;
       default:
         sendResponse({});
         break;
     }
   });
 }