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 294 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.

+  _alreadyLoaded: Symbol(),
 
   observe: function(subject, topic, data, uri)
   {
     switch (topic)
     {
       case "document-element-inserted":
       {
         let window = subject.defaultView;
         if (!window)
           return;
@@ skipping 22 matching lines @@
           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(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)
           uri = window.location.href;
         if (!shouldAllow(opener, opener.document, "POPUP", uri))
@@ skipping 140 matching lines @@
           let weights = parentNode[property].split(",");
           weights[index] = "0";
           parentNode[property] = weights.join(",");
         }
       }
       else
         node.classList.add(cls);
     }
   });
 }