712975 - Tap to activate plugin doesn't work after flipping pref until you restart

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Verdi [:verdi] Best to slack me]

For the most part, tapping to activate plugin doesn't do anything at all. The first time I tried it though, my phone (HTC Desire HD) restarted.

I changed my preference to "Enable plugins: Yes" and embeded YouTube videos appear but tapping on them doesn't do anything.

I changed my preference to "Enable plugins: No" and I still see "Tap here to activate plugin" messages which don't do anything.

Comment 1

[Cristian Nicolae (:xti)]

Have you installed the Adobe Flash Player 11 from the Android Market/update it?

Comment 2

[Verdi [:verdi] Best to slack me]

(In reply to Cristian Nicolae (:xti) from comment #1)
> Have you installed the Adobe Flash Player 11 from the Android Market/update
> it?

Yes. Installed and up-to-date prior to this.

Comment 3

[Cristian Nicolae (:xti)]

I'm able to reproduce this on the latest Nightly build. It seems that 4 of the flash contents are working fine for me (I didn't have to tap on the content to play even if the option was set to "Tap to play").

--
Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20111222
Firefox/12.0a1 Fennec/12.0a1
Devices: Samsung Galaxy S
OS: Android 2.2

Comment 4

[:Margaret Leibovic]

(In reply to Verdi from comment #0)
> For the most part, tapping to activate plugin doesn't do anything at all.
> The first time I tried it though, my phone (HTC Desire HD) restarted.

Were you testing this before bug 710885 was fixed?

> I changed my preference to "Enable plugins: Yes" and embeded YouTube videos
> appear but tapping on them doesn't do anything.
> 
> I changed my preference to "Enable plugins: No" and I still see "Tap here to
> activate plugin" messages which don't do anything.

After changing this setting each time did you restart the browser (quit/reopen)? The preference doesn't go into effect until you restart.

Comment 5

[:Margaret Leibovic]

Maybe we need to add some message about how changing the setting doesn't go into effect until the browser restarts, since I can see users getting confused about this.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Margaret Leibovic [:margaret] from comment #5)
> Maybe we need to add some message about how changing the setting doesn't go
> into effect until the browser restarts, since I can see users getting
> confused about this.

or we can put a listener on the preference so it does have immediate effect

Comment 7

[Aaron Train [:aaronmt]]

Regardless; his embedded YouTube video's won't play with 'always-run'. Is that seperate issue a dupe of a wont-fix?

Comment 8

[Verdi [:verdi] Best to slack me]

(In reply to Margaret Leibovic [:margaret] from comment #4)
> (In reply to Verdi from comment #0)
> > For the most part, tapping to activate plugin doesn't do anything at all.
> > The first time I tried it though, my phone (HTC Desire HD) restarted.
> 
> Were you testing this before bug 710885 was fixed?

I don't think so. I tested it with today's new native aurora build that the Mobile Test Drivers email said to install this morning.

> 
> > I changed my preference to "Enable plugins: Yes" and embeded YouTube videos
> > appear but tapping on them doesn't do anything.
> > 
> > I changed my preference to "Enable plugins: No" and I still see "Tap here to
> > activate plugin" messages which don't do anything.
> 
> After changing this setting each time did you restart the browser
> (quit/reopen)? The preference doesn't go into effect until you restart.

No I didn't restart. I had no idea that was required. None of the other preferences seem to (or say) require a restart.

Comment 9

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #7)
> Regardless; his embedded YouTube video's won't play with 'always-run'. Is
> that seperate issue a dupe of a wont-fix?

I wanted to check about whether he had restarted before experiencing that issue. It's possible that he flipped the pref to disable flash, restarted, then flipped the pref to always play and found it wasn't working before he restarted again. If it doesn't play with the pref correctly set, that's probably a separate issue.

(In reply to Verdi from comment #8)
> (In reply to Margaret Leibovic [:margaret] from comment #4)
> > (In reply to Verdi from comment #0)
> > > For the most part, tapping to activate plugin doesn't do anything at all.
> > > The first time I tried it though, my phone (HTC Desire HD) restarted.
> > 
> > Were you testing this before bug 710885 was fixed?
> 
> I don't think so. I tested it with today's new native aurora build that the
> Mobile Test Drivers email said to install this morning.

Okay, so just to be clear, if you set the pref to click to play, then quit and reopen the browser, does tap to play work as expected? If not, we should file a new bug about that.

> No I didn't restart. I had no idea that was required. None of the other
> preferences seem to (or say) require a restart.

Yeah, that's bad. I think this bug should just be about the unexpected behavior that ensues after you flip the pref and no changes go into effect.

Sorry for all this confusion! I hope we're on the same page now.

Comment 10

[Verdi [:verdi] Best to slack me]

Ok I thought this might have something to do with the embed being an iframe so I set up this simple page to test:
http://people.mozilla.org/~mverdi/mockups/youtube.html

Here's what I found (restarting between each change):
1. Plugins turned on: Both videos appeared as regular YouTube embeded videos. The iframe video would not play, the old embed video played when tapped.
2. Plugins set to tap to play: The iframe video appeared as a regular YouTube embeded video and would not play. The old embed video had a tap to play message but would not play when tapped.
3. Plugins set to off: Again the iframe video appeared as a regular YouTube embeded video and would not play. The old embed video had a plugins needed message.

Comment 11

[:Margaret Leibovic]

(In reply to Verdi from comment #10)
> Ok I thought this might have something to do with the embed being an iframe
> so I set up this simple page to test:
> http://people.mozilla.org/~mverdi/mockups/youtube.html

Thanks for making this page. I verified that the same things happened for me. The issues with taps not being handled correctly in iframes is bug 710704, so that's a known issue.

> 2. Plugins set to tap to play: The iframe video appeared as a regular
> YouTube embeded video and would not play. The old embed video had a tap to
> play message but would not play when tapped.

This is an unfiled bug. I thought bug 710885 would take care of this, but I guess not. Could you file a new bug about that? I will have to look into it. I wonder if it's another issue with click events not getting through.

Comment 12

[Verdi [:verdi] Best to slack me]

(In reply to Margaret Leibovic [:margaret] from comment #11)
> This is an unfiled bug. I thought bug 710885 would take care of this, but I
> guess not. Could you file a new bug about that? I will have to look into it.
> I wonder if it's another issue with click events not getting through.

Filed bug 713080

Comment 13

[:Margaret Leibovic]

Attachment: WIP

This is my attempt at Brad's idea from comment 6. I mostly copied an old WIP patch from bug 189378 to try to add an observer to make plugin.disable live. However, I'm getting a crash after I'm unloading the plugins if there's already a page open that's using plugins, and I haven't figured out how to fix that.

plugins.click_to_play is read every time a plugin object is created, so we don't need to do anything to make that live.

Comment 15

[Josh Aas]

Comment on attachment 586262 [details] [diff] [review]
WIP

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

Do we really need to shut down already-running plugins after you flip the pref? If so, you can see how this is done properly in nsPluginHost::Destroy, where we destroy all running instances, then we unload the plugins themselves, then we cut loose the nsPluginTag objects (the only part your patch currently does). If you don't allow instances and plugins to shut down properly before cutting loose their underlying objects you'll likely crash.

Comment 16

[:Margaret Leibovic]

(In reply to Josh Aas (Mozilla Corporation) from comment #15)
> Do we really need to shut down already-running plugins after you flip the
> pref?

I found that when I didn't shut down already-running plugins, we still loaded plugin content on new pages, so unless there's a better way to prevent this, I think we still need to shut down the plugins.

> If so, you can see how this is done properly in nsPluginHost::Destroy,
> where we destroy all running instances, then we unload the plugins
> themselves, then we cut loose the nsPluginTag objects (the only part your
> patch currently does).

I changed my patch to do this, and now it's successfully unloading the plugins without crashing, but the LoadPlugins() call I added doesn't seem to successfully load them again. Is there something else I need to be doing?

Comment 18

[Aaron Train [:aaronmt]]

Looks like this completely fell off the radar. Should this block 1.0?

Comment 19

[Brian Nicholson (:bnicholson)]

Attachment: patch

Here's the modified patch that shuts down plugins when disabling the pref and loads them when the pref is enabled. Since (I think) we want to do the same thing when disabling the pref and when we destroy, I just called Destroy() directly.

Comment 20

[Josh Aas]

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

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

I'm not thrilled about the name of nsPluginHost::Destroy when it's used this way. It's not really destroying the plugin host, it's just unloading all plugins and their instances. Can you please rename it to nsPluginHost::UnloadPlugins?

Once you've done that then the concepts of "destroyed" and "plugins loaded" are unified, so we don't need both "mPluginsLoaded" and "mIsDestroyed". Get rid of the latter and set "mPluginsLoaded" as appropriate from nsPluginHost::LoadPlugins and nsPluginHost::UnloadPlugins.

All of this will allow you to write cleaner logic in the pref observer, simply:

if (mPluginsDisabled) {
  UnloadPlugins();
} else {
  LoadPlugins();
}

Comment 21

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

Thanks, good idea. Here's an updated patch with the suggested changes.

Comment 22

[Josh Aas]

Comment on attachment 617695 [details] [diff] [review]
patch v2

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

Please remove mIsDestroyed from the class entirely (the header file), otherwise this looks good. Thanks!

Comment 23

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

Patch updated from comment 22 and updated username. r+'d by josh.

http://hg.mozilla.org/integration/mozilla-inbound/rev/d4e74ea14127

Comment 24

[(no longer active)]

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

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 617936 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): none, new feature
User impact if declined: enabling/disabling plugins require browser restart
Testing completed (on m-c, etc.): on m-c for 2 days
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: none

Comment 26

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/73698c7e0af2

Comment 27

[Tony Chung [:tchung]]

Verified fixed on a 5-2 tinderbox aurora build.  no restart necessary to switch from enabled to tap-to-play. 

ftp://ftp.mozilla.org/pub/mobile/tinderbox-builds/mozilla-aurora-android/1335995418/

Comment 28

[Martijn Wargers (dead)]

I tested this on trunk too, works fine there also.
Setting Plugins to disabled doesn't work well, though, I filed bug 750790 for that.

Comment 29

[:Margaret Leibovic]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #28)
> I tested this on trunk too, works fine there also.
> Setting Plugins to disabled doesn't work well, though, I filed bug 750790
> for that.

That's unfortunate... switching between tap to play/enabled was already a live switch before this patch. Thanks for filing, we can investigate in that bug.

Comment 30

[Brian Nicholson (:bnicholson)]

Part of this patch got lost in a merge to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/e1acc0e92a40#l59.117

ehsan has no explanation, so I'm going to reland the missing piece.

Comment 31

[(no longer active)]

Brian, thinking more about this, it's really scary that your patch got reverted.  Can you please make sure that nobody had backed out your patch in any side of the merge?  We rely on the fact that merging works correctly pretty heavily...

Comment 32

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/65795132a36e

Comment 33

[Matt Brubeck (:mbrubeck)]

(In reply to Ehsan Akhgari [:ehsan] from comment #31)
> Brian, thinking more about this, it's really scary that your patch got
> reverted.  Can you please make sure that nobody had backed out your patch in
> any side of the merge?  We rely on the fact that merging works correctly
> pretty heavily...

It looks like the problem was from merging Brian's patch (which originally landed on inbound) with this patch from bug 722942 (which originally landed on m-c):
https://hg.mozilla.org/integration/mozilla-inbound/rev/e18846bc6b20#l4.48

That merge should definitely have generated a conflict and required manual resolution.

Comment 34

[(no longer active)]

I'm 100% sure that I have not manually resolved a conflict in this code.  :/

Comment 35

[Ed Morley [:emorley]]

(In reply to Brian Nicholson (:bnicholson) from comment #32)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/65795132a36e

https://hg.mozilla.org/mozilla-central/rev/65795132a36e

Comment 36

[Paul Feher]

Verified/Fixed on:
Nightly Fennec 15.0a1 (2012-06-18)
Aurora Fennec 14.0a2 (2012-06-17)
Device: HTC Desire Z
OS: Android 2.3.3

There's no need to restart fennec after enabling/disabling plugins.