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

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Adrian Tamas (:AdrianT)]

Good build: June 12
Bad build: June 13 964b11fea7f1

Possible regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=131961e5e0d1&tochange=964b11fea7f1

Comment 2

[Martijn Wargers (dead)]

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.

Comment 3

[:Margaret Leibovic]

This is probably a regression from all the refactoring that's been happening. I'll investigate.

Comment 4

[:Margaret Leibovic]

This was caused by bug 753102. Patch forthcoming.

Comment 5

[:Margaret Leibovic]

Attachment: 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.

Comment 6

[:Margaret Leibovic]

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.

Comment 7

[:Margaret Leibovic]

Attachment: (cleanup) remove hidePlugins() because it isn't used

In my investigation, I found an unused method. Let's get rid of it.

Comment 8

[:Margaret Leibovic]

This issue is present in aurora as well.

Comment 9

[Wesley Johnston (:wesj)]

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.

Comment 10

[Wesley Johnston (:wesj)]

Also, tests?

Comment 11

[:Margaret Leibovic]

Attachment: 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.

Comment 12

[:Margaret Leibovic]

Comment on attachment 639409 [details] [diff] [review]
patch

Wes is on PTO, but I want to land this. Switching review request...

Comment 13

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/485fecad1775
https://hg.mozilla.org/integration/mozilla-inbound/rev/b17d35c73185

Comment 14

[:Margaret Leibovic]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/485fecad1775
https://hg.mozilla.org/mozilla-central/rev/b17d35c73185

Comment 16

[:Margaret Leibovic]

https://hg.mozilla.org/releases/mozilla-aurora/rev/23096b5a0eda