1370018 - Pop-up blocker notification and icon can disappear quickly if popups are created from a subframe / in multi-frame documents

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Loic]

STR:

1) Check "Block pop-up windows" in the options
2) Open https://jsfiddle.net/peo3x4aq/

AR:
The pop-up blocker notification and icon (in the location bar) disappear quickly and the user is not able to click on the button to open the pop-up blocker options.

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4c987b7ed54a630a7de76adcc2eb00dab49d5dfd&tochange=f9acfdca68a45c8cecf54f5a1bfc3b2a4baa52a3

Gijs Kruitbosch — Bug 1325841 - fix hiding popup icon when navigating the browser, r=Felipe

Comment 1

[Gerry Chang [:gchang]]

Track 54+/55+ as regression.

Comment 2

[:Gijs (away/busy until Mar 10)]

This is clearly an attempt at a minimal testcase of some sort given something that happens in some other site. I am a bit confused because if I open only the resulting frame, on its own, ie if I click this link:

https://fiddle.jshell.net/peo3x4aq/show/

then it works just fine.

Specifically, it seems the popup blocker doesn't cope well with multi-frame documents right now. It looks like if you create a popup, and then pagehide fires for a frame, we remove the popups for the entire toplevel browser.

We should only care about pagehide events for the toplevel document, I think. Felipe, does that sound right?

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Gijs from comment #2)
> This is clearly an attempt at a minimal testcase of some sort given
> something that happens in some other site. I am a bit confused because if I
> open only the resulting frame, on its own, ie if I click this link:
> 
> https://fiddle.jshell.net/peo3x4aq/show/
> 
> then it works just fine.
> 
> Specifically, it seems the popup blocker doesn't cope well with multi-frame
> documents right now. It looks like if you create a popup, and then pagehide
> fires for a frame, we remove the popups for the entire toplevel browser.
> 
> We should only care about pagehide events for the toplevel document, I
> think. Felipe, does that sound right?

I guess that makes sense. If it's just the problem that the subframe pagehide makes the icon go away, then it looks right to me. I wonder if there's something worse broken here, i.e. not being able to track properly where each popup came from, and reopening it incorrectly if it's from a subframe (when the user chooses to open it).

Comment 4

[:Gijs (away/busy until Mar 10)]

Realistically, this isn't going to make 54.

Comment 5

[:Gijs (away/busy until Mar 10)]

Attachment: Bug 1370018 - pagehide shouldn't junk all popup data indiscriminately,

Review commit: https://reviewboard.mozilla.org/r/148592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/148592/

Comment 6

[:Gijs (away/busy until Mar 10)]

Comment on attachment 8877217 [details]
Bug 1370018 - pagehide shouldn't junk all popup data indiscriminately,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/148592/diff/1-2/

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8877217 [details]
Bug 1370018 - pagehide shouldn't junk all popup data indiscriminately,

https://reviewboard.mozilla.org/r/148592/#review153542

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e859dc918b2c
pagehide shouldn't junk all popup data indiscriminately, r=Felipe

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[:Gijs (away/busy until Mar 10)]

Comment on attachment 8877217 [details]
Bug 1370018 - pagehide shouldn't junk all popup data indiscriminately,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1325841
[User impact if declined]: can't allow popups in some circumstances
[Is this code covered by automated tests?]: yep, added tests in this patch
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: probably, see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not really
[Why is the change risky/not risky?]: it's a relatively small change, it's net code simplification, there's automated tests.
[String changes made/needed]: nope

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8877217 [details]
Bug 1370018 - pagehide shouldn't junk all popup data indiscriminately,

popup blocker fix, beta55+

I confirmed that I can reproduce this in 55.0b1 but not in nightly 2017-06-15.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/25bf27f7c252

Comment 13

[Ryan VanderMeulen [:RyanVM]]

If we wanted to uplift this to ESR52, we'll need a rebased patch. Worth the effort or should we just wontfix?

Comment 14

[Gerry Chang [:gchang]]

We can wontfix it for ESR52.

Comment 15

[:Gijs (away/busy until Mar 10)]

(In reply to Gerry Chang [:gchang] from comment #14)
> We can wontfix it for ESR52.

+1