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

Unfortunately, this got more complicated than it should be. Three things happened here:

1) content-document-global-created notification replaced by document-element-inserted, the latter fires when the document is definitely initialized but still before any scripts run. As a result, the subject parameter is a document now, no longer a window - and it needs to be verified that we have a content window (as opposed to chrome windows which we should not touch).
2) Listening to DOMContentLoaded on pop-up documents to make sure that we "forget" them once that initial load happens. Any further loads should no longer be considered for pop-up blocking.
3) Since window.opener is still there even after the window loads, we cannot simply remove the window from _openers - the information will be recovered from window.opener when the user navigates to a different page. So we need to store a dummy value indicating "don`t touch" instead.

Repository: hg.adblockplus.org/adblockplus

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

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
lib/child/contentPolicy.js:315: _alreadyLoaded: {},
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.

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
lib/child/contentPolicy.js:358: let forgetPopup = event =>
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]

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
lib/child/contentPolicy.js:315: _alreadyLoaded: {},
On 2016/08/16 09:50:25, Thomas Greiner wrote:
> 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.

Done.

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
lib/child/contentPolicy.js:358: let forgetPopup = event =>
On 2016/08/16 09:50:25, Thomas Greiner wrote:
> 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.

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]

LGTM

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
File lib/child/contentPolicy.js (right):

https://codereview.adblockplus.org/29349811/diff/29349814/lib/child/contentPo...
lib/child/contentPolicy.js:358: let forgetPopup = event =>
On 2016/08/16 10:36:05, Wladimir Palant wrote:
> On 2016/08/16 09:50:25, Thomas Greiner wrote:
> > 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.
> 
> 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.

I agree, that approach makes more sense.