Issue 1393 - Block element dialog suggests decoded URLs

include.postload.js

Index: include.postload.js
===================================================================
--- a/include.postload.js
+++ b/include.postload.js
@@ -279,11 +279,6 @@
   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;
@@ -349,8 +344,11 @@
       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;
 }