1054840 - Unexpected popup blocker notification pops up in Firefox31, Aurora33.0a2 and Nightly34.0a1

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Alice0775 White]

Attachment: popup.html

[Tracking Requested - why for this release]: regression

This problem happens on Firefox31, Aurora33.0a2 and Nightly34.0a1.
This problem does not happen on Firefox30, 32.0b7, IE11 and Chrome36.

Steps To Reproduce:
1. Open attached
2. Open any other page

Actual Results:
Unexpected popup blocker notification pops up

Expected Results:
Popup blocker notification should not pop up


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbed599ec413
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140320161729
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/257d152e487f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140320164730
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dbed599ec413&tochange=257d152e487f

Regressed by:
8c129d201f96	Bill McCloskey — Bug 933462 - [e10s] Pop-up blocking notifications (r=felipe)


Pregression window in (aurora32 only)
Bad:
https://hg.mozilla.org/releases/mozilla-aurora/rev/51243afbb1a9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140711202012
Fixed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3f6dbd37993
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140711211411
Pushlog:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=51243afbb1a9&tochange=b3f6dbd37993

Fixed by: 
	b3f6dbd37993	Bill McCloskey — Bug 1024391 - Backout bug 933462 for breaking Android. a=lmandel

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Bill, can you help here?

Tracking because it is a regression and 32 is fixed (while 33 is not)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Related to bug 1047111?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: popup-notification-fix

This is a separate problem. Here's what happens in without the popup code for e10s:

1. Web page asks to open a popup in onunload.
2. We show the popup blocker UI.
3. We get a pagehide event for the page, which nulls out all the popup info and removes the popup UI from the URL bar, but not from the notification box.
3. We hit the onLocationChange code here, which removes the notification box:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4213

Here's what happens with the e10s version:

1. Same.
2. Content script sends a message asking to show popup blocker UI.
3. Content script sends another message after pagehide that nulls out the popup data.
4. We hit onLocationChange, but no notification is showing, so we do nothing.
5. We receive the first message and show the popup UI.
6. We get the second message and remove the UI from the URL bar, but not from the notification box.

So the basic issue is that using messages causes stuff to happen at a different time relative to onLocationChange. This actually shouldn't be a problem in an e10s build because we use messages for everything, including onLocationChange.

I'd like to fix this by removing the notification box when we get the second popup message. I don't see why we don't do this already. It's not really very useful once we've nulled out blockedPopups since we won't be able to show anything anyway.

Comment 6

[:Gijs (he/him)]

Per the dupe and my own experience, this issue also affects 32 beta. Are we sure it was definitely backed out and should be fixed now?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8474882 [details] [diff] [review]
popup-notification-fix

Review of attachment 8474882 [details] [diff] [review]:
-----------------------------------------------------------------

With this, can't we remove the code that removed the notificationbox in OnLocationChange?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Felipe Gomes from comment #7)
> Comment on attachment 8474882 [details] [diff] [review]
> popup-notification-fix
> 
> Review of attachment 8474882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> With this, can't we remove the code that removed the notificationbox in
> OnLocationChange?

I think that code is responsible for all notifications, not just the popup blocker ones. So I think we still need it.

Also, this is not a dupe of bug 1055197 since it doesn't affect the URL bar icon--only the notification box.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43167be71864

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8474882 [details] [diff] [review]
popup-notification-fix

Approval Request Comment
[Feature/regressing bug #]: bug 933462
[User impact if declined]: Weird popup blocking behavior if popup is shown when navigating away from site.
[Describe test coverage new/current, TBPL]: On m-c.
[Risks and why]: There is some risk of regressions, but not much.
[String/UUID change made/needed]: None

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/43167be71864

Comment 12

[Sylvestre Ledru [:Sylvestre]]

I don't think we will take this patch for esr.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Per comment 6, Gijs is able to repro this issue on 32. Does 32 require this fix, which looks safe enough? If so, we'll want to land this by tomorrow (Aug 21).

Comment 14

[:Gijs (he/him)]

(In reply to Lawrence Mandel [:lmandel] from comment #13)
> Per comment 6, Gijs is able to repro this issue on 32. Does 32 require this
> fix, which looks safe enough? If so, we'll want to land this by tomorrow
> (Aug 21).

I'm sorry for the confusion. I was confused by the very similar-sounding issues. The issue I was experiencing is bug 1055197, which in turn is apparently a dupe of bug 1045826 instead of this one. See comment #8.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d3ff71d97d36

Comment 16

[Bogdan Maris, Desktop Test Engineering]

Using latest Aurora and Firefox 33 beta 2 (not e10s) the pop-up notification is displayed until the website loads and then it disappears. Using e10s new window I can`t see the pop-up so the issue is fixed but only on e10s. For the non e10s windows is this the intended behavior?

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think the current behavior is fine. This is a pretty weird corner case.

Comment 18

[Bogdan Maris, Desktop Test Engineering]

Thanks Bill, given comment 17 I`m marking this issues as verified fixed based on the testing from comment 16.