Issue 4335 - Adblock Plus should forget pop-ups after the initial load

lib/child/contentPolicy.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 229 matching lines @@
       if (name.indexOf("TYPE_") == 0 && name != "TYPE_DATAREQUEST")
         types.set(iface[name], name.substr(5));
 
     let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
     registrar.registerFactory(this.classID, this.classDescription, this.contractID, this);
 
     let catMan = Utils.categoryManager;
     for (let category of this.xpcom_categories)
       catMan.addCategoryEntry(category, this.contractID, this.contractID, false, true);
 
-    Services.obs.addObserver(this, "content-document-global-created", true);
+    Services.obs.addObserver(this, "document-element-inserted", true);
 
     onShutdown.add(() =>
     {
-      Services.obs.removeObserver(this, "content-document-global-created");
+      Services.obs.removeObserver(this, "document-element-inserted");
 
       for (let category of this.xpcom_categories)
         catMan.deleteCategoryEntry(category, this.contractID, false);
 
       registrar.unregisterFactory(this.classID, this);
     });
   },
 
   //
   // nsISupports interface implementation
@@ skipping 40 matching lines @@
 
   shouldProcess: function(contentType, contentLocation, requestOrigin, insecNode, mimeType, extra)
   {
     return Ci.nsIContentPolicy.ACCEPT;
   },
 
   //
   // nsIObserver interface implementation
   //
   _openers: new WeakMap(),
+  _alreadyLoaded: {},

[Thomas Greiner, 2016/08/16 09:50:25]

Detail: I'd rather use `Symbol` for that to avoid the impression that we may use
this object for a different purpose than as a mere placeholder. It would also
emphasize that this isn't a valid window.

[Wladimir Palant, 2016/08/16 10:36:05]

Done.

 
   observe: function(subject, topic, data, uri)
   {
     switch (topic)
     {
-      case "content-document-global-created":
+      case "document-element-inserted":
       {
-        let opener = this._openers.get(subject);
-        if (opener && Components.utils.isDeadWrapper(opener))
+        let window = subject.defaultView;
+        if (!window)
+          return;
+
+        let type = window.QueryInterface(Ci.nsIInterfaceRequestor)
+                         .getInterface(Ci.nsIWebNavigation)
+                         .QueryInterface(Ci.nsIDocShellTreeItem)
+                         .itemType;
+        if (type != Ci.nsIDocShellTreeItem.typeContent)
+          return;
+
+        let opener = this._openers.get(window);
+        if (opener == this._alreadyLoaded)
+        {
+          // This window has loaded already, ignore it regardless of whether
+          // window.opener is still set.
+          return;
+        }
+
+        if (opener && Cu.isDeadWrapper(opener))
           opener = null;
 
         if (!opener)
         {
           // We don't know the opener for this window yet, try to find it
-          if (subject instanceof Ci.nsIDOMWindow)
-            opener = subject.opener;
-
+          opener = window.opener;
           if (!opener)
             return;
 
           // The opener might be an intermediate window, get the real one
           while (opener.location == "about:blank" && opener.opener)
             opener = opener.opener;
 
-          this._openers.set(subject, opener);
+          this._openers.set(window, opener);
+
+          let forgetPopup = event =>

[Thomas Greiner, 2016/08/16 09:50:25]

The issue I'm seeing with this approach is that a non-user initiated navigation
after the DOM creation (e.g. an asynchronously executed JavaScript redirect)
would no longer be caught by `$popup` filters.

I'm not quite sure how we could differentiate those though. Determining whether
the navigation was user-initiated might help, if that's possible.

[Wladimir Palant, 2016/08/16 10:36:05]

Yes, we discussed that yesterday briefly. A website can always disguise
automated navigation as a user action - e.g. us adding a timeout here won't help
much. On the other hand, going with "once a pop-up - always a pop-up" is a
perfect recipe for weird false positives and confusing behavior (see steps to
reproduce in the issue for example). So this needs to be addressed via
https://issues.adblockplus.org/ticket/2095, a generic mechanism to disallow
certain navigation, not limited to pop-ups.

[Thomas Greiner, 2016/08/16 11:31:54]

I agree, that approach makes more sense.

+          {
+            subject.removeEventListener("DOMContentLoaded", forgetPopup);
+            this._openers.set(window, this._alreadyLoaded);
+          };
+          subject.addEventListener("DOMContentLoaded", forgetPopup);
         }
 
-        if (!uri && subject instanceof Ci.nsIDOMWindow)
-          uri = subject.location.href;
+        if (!uri)
+          uri = window.location.href;
         if (!shouldAllow(opener, opener.document, "POPUP", uri))
         {
-          subject.stop();
-          Utils.runAsync(() => subject.close());
+          window.stop();
+          Utils.runAsync(() => window.close());
         }
         else if (uri == "about:blank")
         {
           // An about:blank pop-up most likely means that a load will be
           // initiated asynchronously. Wait for that.
           Utils.runAsync(() =>
           {
-            let channel = subject.QueryInterface(Ci.nsIInterfaceRequestor)
-                                 .getInterface(Ci.nsIDocShell)
-                                 .QueryInterface(Ci.nsIDocumentLoader)
-                                 .documentChannel;
+            let channel = window.QueryInterface(Ci.nsIInterfaceRequestor)
+                                .getInterface(Ci.nsIDocShell)
+                                .QueryInterface(Ci.nsIDocumentLoader)
+                                .documentChannel;
             if (channel)
               this.observe(subject, topic, data, channel.URI.spec);
           });
         }
         break;
       }
     }
   },
 
   //
@@ skipping 18 matching lines @@
         return;
 
       if (contentType == Ci.nsIContentPolicy.TYPE_DOCUMENT)
       {
         if (wnd.history.length <= 1 && wnd.opener)
         {
           // Special treatment for pop-up windows - this will close the window
           // rather than preventing the redirect. Note that we might not have
           // seen the original channel yet because the redirect happened before
           // the async code in observe() had a chance to run.
-          this.observe(wnd, "content-document-global-created", null, oldChannel.URI.spec);
-          this.observe(wnd, "content-document-global-created", null, newChannel.URI.spec);
+          this.observe(wnd.document, "document-element-inserted", null, oldChannel.URI.spec);
+          this.observe(wnd.document, "document-element-inserted", null, newChannel.URI.spec);
         }
         return;
       }
 
       shouldAllowAsync(wnd, wnd.document, types.get(contentType), newChannel.URI.spec, function(allow)
       {
         callback.onRedirectVerifyCallback(allow ? Cr.NS_OK : Cr.NS_BINDING_ABORTED);
       });
       async = true;
     }
@@ skipping 76 matching lines @@
           let weights = parentNode[property].split(",");
           weights[index] = "0";
           parentNode[property] = weights.join(",");
         }
       }
       else
         node.classList.add(cls);
     }
   });
 }