Doorhangers in the selected tab reappear when background tabs fire change events

VERIFIED FIXED in Firefox 15

Status

()

Firefox for Android
General
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: Margaret)

Tracking

({regression, testcase})

Trunk
Firefox 16
ARM
Android
regression, testcase
Points:
---

Firefox Tracking Flags

(firefox15+ verified, firefox16 verified, firefox17 verified)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Make sure you have Plugins set to "Tap to Play" in your settings

Steps to reproduce:
- Open testcase, plugin doorhanger should appear (just dismiss it by tapping outside of it)
- Long tap on one of the links at the bottom
- In the resulting context menu, tap on the "Open Link in New Tab".

Expected result:
- No plugin doorhanger prompt appears.

Actual result:
- Pluing doorhanger prompt appears.

I've seen this bug in current trunk build, but not in Aurora, nor in beta.
Good build: June 12
Bad build: June 13 964b11fea7f1

Possible regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=131961e5e0d1&tochange=964b11fea7f1
Keywords: regressionwindow-wanted
(Reporter)

Comment 2

6 years ago
Thanks, Adrian.
On inbound, the regression range is: from 4a4519b018e9 to d846a8245199
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?startdate=2012-06-12+00%3A00%3A00&enddate=2012-06-13+02%3A00%3A00

As usual, with all these checkins, it is impossible for me which caused the regression.
(Assignee)

Comment 3

6 years ago
This is probably a regression from all the refactoring that's been happening. I'll investigate.
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 4

6 years ago
This was caused by bug 753102. Patch forthcoming.
Blocks: 753102
(Assignee)

Comment 5

6 years ago
Created attachment 638884 [details] [diff] [review]
patch

The problem that caused this bug is that we're calling updatePopups() on background tabs. The same problem happens if you close a background tab.

All the methods that are getting called in GeckoApp's onTabsChanged method only apply to the selected tab, but we only check for that in some of them. This patch moves that check to the top and bails if the event isn't about the selected tab.
Attachment #638884 - Flags: review?(wjohnston)
(Assignee)

Comment 6

6 years ago
Actually, we may need to call hidePlugins(tab) on a tab when it's closed. Snorp, is that true, or do we only care about calling hidePlugins on the selected tab? I feel like there's probably other code paths that just totally destroy the plugins on a closed tab.
(Assignee)

Comment 7

6 years ago
Created attachment 638886 [details] [diff] [review]
(cleanup) remove hidePlugins() because it isn't used

In my investigation, I found an unused method. Let's get rid of it.
Attachment #638886 - Flags: review?(snorp)
(Assignee)

Comment 8

6 years ago
This issue is present in aurora as well.
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox15: --- → ?
(Assignee)

Updated

6 years ago
Summary: Plugin doorhanger prompt reappears after opening a link in new tab → Doorhangers in the selected tab reappear when background tabs fire change events
Attachment #638886 - Flags: review?(snorp) → review+
Comment on attachment 638884 [details] [diff] [review]
patch

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

Thanks! In order to avoid changing behavior, I tried to keep these separate if possible, but I'm fine with this. Probably needs uplift to Aurora as well.
Attachment #638884 - Flags: review?(wjohnston) → review+
Also, tests?
(Assignee)

Comment 11

6 years ago
Created attachment 639409 [details] [diff] [review]
patch

So, I was thinking about this more, and I realized that other patch was wrong :/

First of all, we'll never get an UNSELECTED event for the currently selected tab (duh). Also, we don't need to be listening for CLOSED events, since we'll always get an UNSELECTED first. And we don't need to listen for ADDED either, since we'll always get a SELECTED right afterwards if that new tab is the selected tab.
Attachment #638884 - Attachment is obsolete: true
Attachment #639409 - Flags: review?(wjohnston)
(Assignee)

Comment 12

6 years ago
Comment on attachment 639409 [details] [diff] [review]
patch

Wes is on PTO, but I want to land this. Switching review request...
Attachment #639409 - Flags: review?(wjohnston) → review?(mark.finkle)
Attachment #639409 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 14

6 years ago
Comment on attachment 639409 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression caused by bug 753102
User impact if declined: doorhangers can unexpectedly reappear; this patch also gets rid of some extra unnecessary work
Testing completed (on m-c, etc.): just landed on inbound
Risk to taking this patch (and alternatives if risky): just simplifies logic, should be low-risk
String or UUID changes made by this patch: n/a
Attachment #639409 - Flags: approval-mozilla-aurora?

Updated

6 years ago
tracking-firefox15: ? → +

Updated

6 years ago
Attachment #639409 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/23096b5a0eda
status-firefox15: affected → fixed
status-firefox16: affected → fixed

Updated

6 years ago
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
status-firefox16: fixed → verified
status-firefox17: --- → verified
You need to log in before you can comment on or make changes to this bug.