Closed Bug 1054840 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: General, defect)

31 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 34
Tracking Status
firefox31 --- wontfix
firefox32 --- unaffected
firefox33 + verified
firefox34 + verified
firefox-esr24 --- unaffected
firefox-esr31 - wontfix

People

(Reporter: alice0775, Assigned: billm)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

Attached file 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
[Tracking Requested - why for this release]:
Bill, can you help here?

Tracking because it is a regression and 32 is fixed (while 33 is not)
Flags: needinfo?(wmccloskey)
Related to bug 1047111?
Depends on: 1047111
No longer depends on: 1024391
No longer depends on: 1047111
Flags: needinfo?(wmccloskey)
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8474882 - Flags: review?(felipc)
Duplicate of this bug: 1055197
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?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(alice0775)
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?
(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.
Flags: needinfo?(wmccloskey)
Attachment #8474882 - Flags: review?(felipc) → review+
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
Attachment #8474882 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/43167be71864
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
I don't think we will take this patch for esr.
Attachment #8474882 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
(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.
Flags: needinfo?(alice0775)
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?
Flags: needinfo?(wmccloskey)
I think the current behavior is fine. This is a pretty weird corner case.
Flags: needinfo?(wmccloskey)
Thanks Bill, given comment 17 I`m marking this issues as verified fixed based on the testing from comment 16.
Status: RESOLVED → VERIFIED
Keywords: qawanted, verifyme
You need to log in before you can comment on or make changes to this bug.