Issue 1396 - Avoid infinite recursion when collapsing <input type="image"> elements

include.preload.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 10 matching lines @@
   "img": "IMAGE",
   "input": "IMAGE",
   "audio": "MEDIA",
   "video": "MEDIA",
   "frame": "SUBDOCUMENT",
   "iframe": "SUBDOCUMENT"
 };
 
 function checkCollapse(element)
 {
+  // <input type="image"> elements try to load their image again
+  // when the "display" CSS property is changed by the
+  // collapsing code below. So we have to bail out, if collapsing
+  // is already in progress, to avoid an infinite recursion.
+  if (element._collapsing)
+    return;

[Wladimir Palant, 2014/09/24 14:57:00]

Interesting, a load triggered for an element with display:none sounds like a
bug. I could confirm that this indeed happens: only for <input type="image"> but
not for <img>, and only if the request was blocked (maybe pointing the request
to a non-existing server will have the same effect). I doubt strongly that other
browsers like Firefox behave the same, sounds worth filing a Chrome bug on this.

Either way, the name of that variable is misleading - this isn't an re-entrance
check, element._collapsing is never removed. It rather marks the elements as
"already collapsed, don't do anything with it." While scenarios where not
collapsing an element a second time will hurt us should be rather rare, IMHO
there is a much simpler and more robust way to avoid infinite recursion: don't
set the element.style.display if its value is already "none."

[Thomas Greiner, 2014/09/24 15:08:29]

That's what I checked before and it does have the same effect indeed.
Unfortunately, the W3C standard doesn't really define a behavior for this case
only "if the fetching process fails without a response from the remote server,
or completes but the image is not a valid or supported image" (see
http://www.w3.org/TR/2012/WD-html5-20121025/states-of-the-type-attribute.html...).

[Sebastian Noack, 2014/09/24 16:42:40]

The only thing the standard defines for that particular case is that an "error"
event must be dispatched.

However, the standard also specifies when the image must be fetched. But allows
to ignore those rules and fetch the image on demand instead, which isn't further
specified. Also it doesn't explicitly forbids to re-fetch images later. So from
what I can tell, Chrome's behavior should be fine.

[Sebastian Noack, 2014/09/24 17:04:58]

Done, though it wasn't actually that simply. I also have to check for the
priority. Otherwise collapsing won't work if the "style" attribute already
specifies "display: none" and an external stylesheet overrides it with
"!important".

+
   var tag = element.localName;
   if (tag in typeMap)
   {
     // This element failed loading, did we block it?
     var url = element.src;
     if (!url)
       return;
 
     ext.backgroundPage.sendMessage(
       {
         type: "should-collapse",
         url: url,
         mediatype: typeMap[tag]
       },
 
       function(response)
       {
         if (response && element.parentNode)
         {
+          element._collapsing = true;

[Thomas Greiner, 2014/09/24 12:35:32]

This makes us quite easily distinguishable from other adblockers. What about
using WeakSet for browsers that support it?

Apart from that, this is an HTMLElement so I'd rather use
`element.dataset.collapsed = true;` to keep it standards conformant.

[Sebastian Noack, 2014/09/24 12:50:09]

No, it doesn't. Content scripts run in a different universe. Custom properties
we add to objects aren't visible to the website. However, if we change DOM
content (that includes using the dataset property) it will be visible to the
website.

[Thomas Greiner, 2014/09/24 14:14:54]

I can confirm that custom properties are not part of the shared DOM.

+
           // <frame> cannot be removed, doing that will mess up the frameset
           if (tag == "frame")
             element.style.setProperty("visibility", "hidden", "important");
           else
             element.style.setProperty("display", "none", "important");
         }
       }
     );
   }
 }
@@ skipping 139 matching lines @@
   }, true);
 
   ext.backgroundPage.sendMessage({type: "get-selectors"}, setElemhideCSSRules);
 }
 
 if (document instanceof HTMLDocument)
 {
   checkSitekey();
   init(document);
 }