1685056 - Popup Blocker bar disappears immediately after window.open is called by a script in an iframe, and that iframe gets removed.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Julian Descottes [:jdescottes]]

From discourse report at https://discourse.mozilla.org/t/targeted-links-issue/73003/12

STRs:

• enable popup blocking
• open https://doubleaction.com/special-orders/
• click on "special orders vendor" link

The event on this link is captured by a script which instead calls window.open. This logically triggers the popup blocker, however the "bar" normally displayed on top of the page disappears immediately, which means the user has no way to allow this popup?

The script responsible for handling the event is at https://doubleaction.com/wp-content/cache/autoptimize/js/autoptimize_c380e1896cbe551ce5d72164a41dc9e2.js

The unminified handler is

  function t(e) {
    var t,
    o,
    i,
    d;
    return e = e || window.event,
    t = e.currentTarget || e.srcElement,
    i = t.getAttribute('href'),
    i && (d = e.ctrlKey || e.shiftKey || e.metaKey, o = t.getAttribute('target'), d || o && !r(o)) ? (n.open(i), e.preventDefault ? e.preventDefault() : e.returnValue = !1, !1) : void 0
  }

n.open(i) is

  u = window.open;
  n.open = function (e, n, t) {
    var o;
    return r(n) ? u.apply(window, arguments) : d ? (o = u.apply(window, arguments), o.opener = null, o) : i(e, n, t)
  },

Comment 1

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

I think this should be allowed with transient activation. Edgar, could you double check? Thanks!

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This looks like a recent-ish regression, the page works in 83.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=416d073941f5f9ca6b7b5262b98eb87100f0b8d0&tochange=68af110cd33e39111ef50541073b3c681a59bc2b

Comment 4

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Broken popup blocker UX.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: test-case, sorta

sorta, since this doesn't work in Chrome either...

It seems they're using this library, which basically injects a display: none iframe and calls window.open() on that iframe, then removes it.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: beautified code from the page.

The reason this works on other browsers on that particular page is... UA-sniffing, of course :(

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Ok, given that, and that the minimized test-case doesn't work on other browsers, I think we need to fix the popup bar to not disappear. I contacted the site so it can hopefully be fixed too. Firefox was never vulnerable to the vulnerabilities blankshield.js protects against, afaict. That library can pretty much be removed at this point.

Comment 8

[Julian Descottes [:jdescottes]]

It seems the sniffing code comes from a pretty popular wordpress plugin https://pluginarchive.com/wordpress/better-wp-security

The code for the plugin is no longer open source, but I downloaded the latest release and could find it under better-wp-security/core/modules/wordpress-tweaks/js/block-tabnapping.js

This plugin is no longer open source and is developed by https://ithemes.com

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: block-tabnapping.js

Original file from the WP plugin with UA sniffing logic.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

FML :-(

So, the other annoying thing, is that fixing the popup blocker UI is not so straight-forward as I hoped.

Turns out we keep the blocked popups on the child process, keyed per window, and only query them when needed. So if the window is gone, it's not like we could retrieve the blocked popup...

Mike, I don't know how important lazily-retrieving the popups is, mind commenting? If we could send them as we blocked them, we could probably just make this a bunch of C++ and just fire PopupBlocked events in the top-level <browser> or something.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

So, I guess, alternatives:

• Hack around this particular case, checking user activation on the parent window if in-process for popup-blocking. This could be the easiest I guess... Slightly unfortunate.
• Backing out the offending patch (maybe only from beta for now?) while we sort this out, either fixing the blocker UI, or something else.
• Some sort of compat intervention.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

Mike, I don't know how important lazily-retrieving the popups is, mind commenting? If we could send them as we blocked them, we could probably just make this a bunch of C++ and just fire PopupBlocked events in the top-level <browser> or something.

I think the consideration here is that we don't want content processes to be able to spam the parent process with PopupBlocked events - see https://searchfox.org/mozilla-central/rev/31ddf859c57e812878a5f817e4097efb06de4d97/toolkit/actors/PopupBlockingChild.jsm#120-123, for example.

If we can ensure that these are throttled, then yes, I can't off the top of my head think of a reason why we couldn't send this stuff to the parent to hold onto.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1685056 - For an initial, same-origin iframe, consume activation on the parent page for multiple-popup blocking. r=edgar,smaug

Sadness. Test times out without the fix.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

I filed bug 1686097 for the confusing UI here, and the issue with iframes and such.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1679456

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac406d1aa9a7
For an initial, same-origin iframe, consume activation on the parent page for multiple-popup blocking. r=edgar

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ac406d1aa9a7

Comment 18

[Julien Cristau [:jcristau]]

Is this suitable for uplift into the last 85 beta?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9196428 [details]
Bug 1685056 - For an initial, same-origin iframe, consume activation on the parent page for multiple-popup blocking. r=edgar,smaug

Beta/Release Uplift Approval Request

• User impact if declined: Comment 0
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Make the popup blocker a bit less restrictive.
• String changes made/needed: none

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 9196428 [details]
Bug 1685056 - For an initial, same-origin iframe, consume activation on the parent page for multiple-popup blocking. r=edgar,smaug

approved for 85.0b9

Comment 21

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32bc039d4091