Closed Bug 712975 Opened 12 years ago Closed 12 years ago

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

Categories

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

ARM
Android
defect

Tracking

(firefox14 verified, firefox15 verified, blocking-fennec1.0 soft, fennec+)

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- soft
fennec + ---

People

(Reporter: verdi, Assigned: bnicholson)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
Have you installed the Adobe Flash Player 11 from the Android Market/update it?
(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.
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
Version: Firefox 11 → Trunk
(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.
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.
(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
Assignee: nobody → margaret.leibovic
Priority: -- → P3
Summary: Tap to activate plugin doesn't work → Tap to activate plugin doesn't work after flipping pref until you restart
Regardless; his embedded YouTube video's won't play with 'always-run'. Is that seperate issue a dupe of a wont-fix?
(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.
(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.
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.
(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.
(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
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #586262 - Flags: feedback?(joshmoz)
tracking-fennec: --- → 11+
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.
Attachment #586262 - Flags: feedback?(joshmoz) → feedback-
(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?
Looks like this completely fell off the radar. Should this block 1.0?
tracking-fennec: 11+ → ?
blocking-fennec1.0: --- → ?
Blocks: 744060
tracking-fennec: ? → +
blocking-fennec1.0: ? → soft
Assignee: margaret.leibovic → bnicholson
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #586262 - Attachment is obsolete: true
Attachment #615900 - Flags: review?(joshmoz)
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();
}
Attachment #615900 - Flags: review?(joshmoz) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Thanks, good idea. Here's an updated patch with the suggested changes.
Attachment #615900 - Attachment is obsolete: true
Attachment #617695 - Flags: review?(joshmoz)
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!
Attachment #617695 - Flags: review?(joshmoz) → review+
Attached patch patch v2Splinter Review
Patch updated from comment 22 and updated username. r+'d by josh.

http://hg.mozilla.org/integration/mozilla-inbound/rev/d4e74ea14127
Attachment #617695 - Attachment is obsolete: true
Attachment #617936 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d4e74ea14127
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
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
Attachment #617936 - Flags: approval-mozilla-aurora?
Attachment #617936 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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/
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.
Status: RESOLVED → VERIFIED
(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.
Depends on: 750790
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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...
(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.
I'm 100% sure that I have not manually resolved a conflict in this code.  :/
(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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.