Issue 581 - Fixed element hiding/collapsing in anonymous frames on Chrome

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 50 matching lines @@
 function checkExceptionKey()
 {
   var attr = document.documentElement.getAttribute("data-adblockkey");
   if (attr)
     ext.backgroundPage.sendMessage({type: "add-key-exception", token: attr});
 }
 
 function hasInlineURL(element, attribute)
 {
   var value = element.getAttribute(attribute);
   return value == null || /^\s*(javascript:|about:|$)/i.test(value);

[Wladimir Palant, 2014/06/17 13:00:24]

I'm surprised you check for null explicitly rather than doing .test(value || "")
:)

Doing it this way actually makes sense here, we want to treat null and empty src
attribute the same.

[Sebastian Noack, 2014/06/17 14:13:48]

I want to skip regex matching if the attribute is missing, which is the case
with almost all <object> tags. And even if the performance advantage is just
theoretical, I don't see how this adds any complexity.

Also we want to treat javascript: and about: urls the same as an empty string.
But you are not suggesting to replace those with an empty string before testing
the string.

 }
 
+function isInlineFrame(element)
+{
+  switch (element.localName)
+  {
+    case "iframe":
+      return hasInlineURL(element, "src") || element.hasAttribute("srcdoc");
+    case "frame":
+      return hasInlineURL(element, "src");
+    case "object":
+      return hasInlineURL(element, "data") && element.contentDocument;
+    default:
+      return false;
+  }
+}
+
 // Converts relative to absolute URL
 // e.g.: foo.swf on http://example.com/whatever/bar.html
 //  -> http://example.com/whatever/foo.swf
 function relativeToAbsoluteUrl(url)
 {
   // If URL is already absolute, don't mess with it
   if (!url || /^[\w\-]+:/i.test(url))
     return url;
 
   // Leading / means absolute path
@@ skipping 103 matching lines @@
   };
 
   document.addEventListener("error", function(event)
   {
     checkCollapse(event.target);
   }, true);
 
   document.addEventListener("load", function(event)
   {
     var element = event.target;
-    var isFrame = /^i?frame$/.test(element.localName);
-
-    if (isFrame)
+
+    if (/^i?frame$/.test(element.localName))
       checkCollapse(element);
 
-    if (fixInlineFrames && (isFrame ? hasInlineURL(element, "src") ||
-                                      element.hasAttribute("srcdoc")
-
-                                    : element.localName == "object" &&
-                                      hasInlineURL(element, "data") &&
-                                      element.documentElement))

[Wladimir Palant, 2014/06/17 13:00:24]

There is at least one more scenario: <embed src="..." type="text/html">

What is element.documentElement? IMHO this property doesn't exist. I guess you
meant to check element.contentDocument?

The logic here is getting complicated enough that it makes sense to simplify it,
e.g. by using a map:

{
  "frame": src,
  "iframe": src,
  "object": data,
  "embed:" src
}

Then you will still have a special case for srcdoc - that one is specific to the
iframe tag however, it isn't allowed on a frame tag (your code is getting this
wrong right now).

[Sebastian Noack, 2014/06/17 14:13:48]

As discussed on IRC, we ignore this case because it is rather exotic and would
add additional complexity since there is no contentDocument property for <embed>
elements.
Ouch, you are right. Fixed.
I don't see how that mapping would make anything less complicated. For iframes
we additionally have to check for srcdoc. For objects we have to make sure that
contentDocument exist. And for embed (if we would handle it) we would have to
get the document from window.frames. So beside the attribute there is still one
more thing that is special in each case. However we can move those check into a
separate function using switch/case.
Good point. Fixed.

+    if (fixInlineFrames && isInlineFrame(element))
     {
       init(element.contentDocument);
 
       for (var tagName in typeMap)
         Array.prototype.forEach.call(element.contentDocument.getElementsByTagName(tagName), checkCollapse);
     }
   }, true);
 
   ext.backgroundPage.sendMessage({type: "get-selectors"}, setElemhideCSSRules);
 }
 
 if (document.documentElement instanceof HTMLElement)
 {
   checkExceptionKey();
   init(document);
 }