STR: 1. Set plugins.click_to_play=TRUE in about:config 2. Go to vk.com 3. Login 4. Go to music section Actual results: The songs doesn't play Expected results: The songs should play

Are you saying that the infobar never appears? Can you construct a more minimal testcase that doesn't involve having an account (or at least a link to a public page which shows the issue)?

Don't know exactly how to do a testcase for this. It's playlist with songs, every songs has a play button next to it, it's not about any infobar. Simply the play button doesn't work. It's something like http://grooveshark.com/#!/search?q=rihanna, the difference here is that grooveshark.com says "We had a problem loading Adobe Flash. You may have a Flash blocker installed. If so, please disable the blocker (or add an exception) and reload to start listening." I'll try to find a similar site.

I think I found something similar: http://www.jango.com/music/Rihanna?l=0

Do you want to morph this to be about jango.com? There is absolutely no guarantee that they have the same root cause. Looking at Jango, the experience is something like this: * Load the page: Flash advertisements show CTP UI. The Flash audio player is not visible. The blocked-plugin UI shows up in the location bar. * Clicking the blocked-plugin UI in the location bar does not cause the video to work. The developer console shows "[10:59:41.454] TypeError: this.sound is undefined @ http://cd09.s3.static.jango.com/javascripts/proto_player_sm2_35836.js:602" The entire page is a frameset and the hidden player embed is contained in a hidden frame of the frameset. Is it possible that we aren't activating plugins in a subframe properly? Or this may just be another version of bug 800018. Paul, can you write a basic testcase with a plugin in a frameset to see whether that's the problem?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > Are you saying that the infobar never appears? Can you construct a more > minimal testcase that doesn't involve having an account (or at least a link > to a public page which shows the issue)? pandora.com is a good example. The infobar never appears. It does not require an account.

(In reply to Michael Coates [:mcoates] from comment #6) > (In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > > Are you saying that the infobar never appears? Can you construct a more > > minimal testcase that doesn't involve having an account (or at least a link > > to a public page which shows the issue)? > > pandora.com is a good example. > The infobar never appears. It does not require an account. What does everyone mean by "infobar"? I'm assuming we're talking about the popup notification that only appears when you click the plugin icon that shows up in the urlbar. If we're talking about the yellow bar that comes down and warns about an out-of-date plugin, that has nothing to do with click-to-play, and we shouldn't be seeing it. Anyway, on pandora, the icon shows up for me in the urlbar, and when I click it and then click "activate all plugins", pandora works fine. Is that not what others are seeing?

This looks like an issue with "SoundManager2" and the way it loads flash. It has special casing for FlashBlock users, but not generic click-to-play, and doesn't wait for its flash applet to load. After activating plugins, this: > document.querySelector("frameset").childNodes[1].contentDocument.querySelector("embed").PercentLoaded() Indicates that the flash applet is running (PercentLoaded is a flash function) -- so soundmanager just needs to account for CTP in its handling of this. Un-minified SoundManager2: https://github.com/scottschiller/SoundManager2/blob/master/script/soundmanager2.js

After today's channel meeting, this will be covered for 18 with bug 810082 but we're not tracking this for 17 - instead we'll not be blocklisting Flash yet in 17 CTP.

(In reply to John Schoenick [:johns] from comment #8) > This looks like an issue with "SoundManager2" and the way it loads flash. It > has special casing for FlashBlock users, but not generic click-to-play, and > doesn't wait for its flash applet to load. > > After activating plugins, this: > > > document.querySelector("frameset").childNodes[1].contentDocument.querySelector("embed").PercentLoaded() > > Indicates that the flash applet is running (PercentLoaded is a flash > function) -- so soundmanager just needs to account for CTP in its handling > of this. > > Un-minified SoundManager2: > https://github.com/scottschiller/SoundManager2/blob/master/script/ > soundmanager2.js Did you file a bug on SoundManager2?

Or perhaps we can do something similar to bug 810082 such that when SoundManager2 is in use, we bring down the Flash enable dialog and reload the page?

I have been in touch with Scott, the author of soundmanager. He has made some changes which should basically fix this case when soundmanager is used correctly. SoundManager is very configurable and jango.com used the worst possible configuration (it explicitly turned off flashblock detection and HTML5 <audio>).

Should be fixed by bug 810082.

vk.com (comment 0) works as expected due to bug 810082. jango.com (comment 4) doesn't work with CTP enabled, but I guess that's ok based on comment 12 ?

(In reply to Paul Silaghi [QA] from comment #14) > vk.com (comment 0) works as expected due to bug 810082. After landing bug 810082, on vk.com, no matter how many times reloading the page, clicking on a song makes the CTP notification appears again, then clicking activate makes the song play. So far so good. Now, after landing bug 819992, CTP notification is limited to once per session. So reloading the page won't show the CTP notification again when clicking on a song. The only way the song will play is to open the CTP doorhanger in the location bar and activate the plugin. The CTP doorhanger is present in the location bar before clicking on a song to play, it's enough to be on vk.com. So here is the problem: if clicking to activate flash from the CTP doorhanger, and then click on a song to play, nothing will happen, the song won't play. The song will play only if clicking on the play button first, and only then activate flash from the CTP doorhanger. What's interesting is that "activate all plugins" instead of "activate flash" and then clicking on a song will make the song play. The big question would be: can this affect other sites as well? Pandora is n/a outside of the US, grooveshark.com works fine, jango.com still broken as I already said in comment 14. To see with your own eyes, feel free to use my test account: user: 40743595269 pass: test123

> So here is the problem: if clicking to activate flash from the CTP doorhanger, and then click on a song to play, nothing will happen, the song won't play. But will it play if you click on the doorhanger a second time, after clicking play? It seems like the site is creating a new flash instance per song, requiring you to activate flash repeatedly - which is not something easy to work around, other than displaying the doorhanger each time (pre-bug 819992)

(In reply to John Schoenick [:johns] from comment #16) > > So here is the problem: if clicking to activate flash from the CTP doorhanger, and then click on a song to play, nothing will happen, the song won't play. > > But will it play if you click on the doorhanger a second time, after > clicking play? There is nothing to click on, the doorhanger is gone after clicking first time.

Reopening according to comment #15. It sounds like the boolean to activate flash plugins is not activating them if they get added to the DOM after the activate button has been pressed. Maybe they are "unknown" during the PluginClickToPlay event and thus aren't activated? This would explain why "Activate All Plugins" allows this workflow to work as expected.

David, can you take a look at this?

Using "Activate All Plugins" sets _clickToPlayPluginsActivated to true. Subsequently, any plugin added to the page that is not blocked by a deny permission will be automatically played in _handleClickToPlayEvent. Using "Activate" on a single type of plugin does not have a similar mechanism. I'll work on a patch for this.

This expands _clickToPlayPluginsActivated to work with multiple types of plugin. This fixes vk.com for me.

Comment on attachment 698063 [details] [diff] [review] patch Review of attachment 698063 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/Makefile.in @@ -248,5 @@ > plugin_test_scriptedNoPopup3.html \ > plugin_alternate_content.html \ > plugin_both.html \ > plugin_both2.html \ > - plugin_bug743421.html \ Why is plugin_bug743421.html being removed here? It is still referenced from browser_bug743421.js. I don't see plugin_add_dynamically.html in this patch.

I hg renamed plugin_bug743421.html to plugin_add_dynamically.html to indicate that that file is used by more than just browser_bug743421.js. And then of course I didn't update that file :-/

Comment on attachment 698763 [details] [diff] [review] patch v2 Review of attachment 698763 [details] [diff] [review]: ----------------------------------------------------------------- Overall everything looks fine, but I think something needs to be done about calling the private methods of PopupNotifications. ::: browser/base/content/test/browser_plugins_added_dynamically.js @@ +68,5 @@ > + gTestBrowser.contentWindow.addPlugin("application/x-second-test"); > + var condition = function() { > + var embeds = gTestBrowser.contentDocument.getElementsByTagName("embed"); > + for (var embed of embeds) { > + var objLoadingContent = embed.QueryInterface(Ci.nsIObjectLoadingContent); I'd prefer to use |let| in new code. @@ +71,5 @@ > + for (var embed of embeds) { > + var objLoadingContent = embed.QueryInterface(Ci.nsIObjectLoadingContent); > + if (embed.type == "application/x-second-test" && objLoadingContent.activated) > + return true; > + } This curly bracket should be un-indented two spaces to line up with "for" @@ +104,5 @@ > + > + // we have to actually show the panel to get the bindings to instantiate > + popupNotification.options.dismissed = false; > + popupNotification.options.eventCallback = testActivateAddSameTypePart3; > + PopupNotifications._showPanel([popupNotification], popupNotification.anchorElement); Is there a way that we can do this without calling private methods of PopupNotifications? This seems too fragile and is unlikely to get noticed if someone later modifies PopupNotifications (until they run the tests). @@ +151,5 @@ > + for (var embed of embeds) { > + var objLoadingContent = embed.QueryInterface(Ci.nsIObjectLoadingContent); > + if (embed.type == "application/x-test" && objLoadingContent.activated) > + return true; > + } This curly bracket needs to be un-indented two spaces to line up with "for". I don't see a functional difference between testActivateAddSameTypePart4 and testActivateAddSameTypePart5. testActivateAddSameTypePart4 will pass once it finds *any* activated embed, whereas testActivateAddSameTypePart5 will pass once it finds that *all* embeds are activated. The failure message for testActivateAddSameTypePart4 says "waited too long for second plugin to activate", but I don't think it will ever get around to checking the second plugin. Since there are no actions between testActivateAddSameTypePart4 and testActivateAddSameTypePart5, I think these two tests can be merged to just check that *all* embeds are activated. ::: browser/base/content/test/plugin_bug743421.html @@ +4,5 @@ > <meta charset="utf-8"> > </head> > <body> > <script> > +function addPlugin(type) { You can use a default argument here to achieve the desired effect. >function addPlugin(type="application/x-text") { See https://blog.mozilla.org/jorendorff/2012/05/29/rest-arguments-and-default-arguments-in-javascript/ for more information.

(In reply to Jared Wein [:jaws] from comment #24) > I don't see a functional difference between testActivateAddSameTypePart4 and > testActivateAddSameTypePart5. testActivateAddSameTypePart4 will pass once it > finds *any* activated embed, whereas testActivateAddSameTypePart5 will pass > once it finds that *all* embeds are activated. The failure message for > testActivateAddSameTypePart4 says "waited too long for second plugin to > activate", but I don't think it will ever get around to checking the second > plugin. Since there are no actions between testActivateAddSameTypePart4 and > testActivateAddSameTypePart5, I think these two tests can be merged to just > check that *all* embeds are activated. Right - I was actually doing the wrong thing in the condition function. It should have checked that the second embed also activated. Anyway, testActivateAddSameTypePart5 is more of a sanity check similar to testActivateAllPart4. The idea is that even if waitForCondition moves on, the test should still check for the expected results instead of just silently succeeding. Does that seem reasonable?

Try run looked good: https://tbpl.mozilla.org/?tree=Try&rev=ac85e88831fc Checkin to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/60f966cc9a72

vk.com works fine now on nightly 21.0a1 (2013-01-22) jango.com doesn't work with CTP enabled, but I guess that's ok based on comment 12 Verified fixed on FF 21. Will this go in FF 19 or 20 ? Please set the flags accordingly.

So, I think this bug got a little confused. We initially were tracking it for various releases, but then it got marked as fixed by bug 810082. This turned out to not be entirely true, so it was fixed in 21. I'm resetting the flags and checking to see if we're going to uplift this to aurora/beta/esr17.

Please nominate for uplift to all branches. We were under the impression that this was resolved in FF18 through bug 810082 in combination with the ability for it to be resolved server-side (comment 12). Is that no longer the case? The whole idea for getting bug 810082 into FF18 was to utilize CTP for Flash before the end of January. We need to understand whether that's still possible.

Here's my understanding: the bug that this patch fixes involves pages adding more flash embeds to a page after a user has clicked in the popup notification to activate plugins. If the user clicked the big "Activate All Plugins", everything works as expected. If the user clicked the "Activate" button next to "Adobe Flash", embeds dynamically added to the page thereafter won't be activated. On a good news/bad news note, after bug 820497 landed in 20, the user can use the popup notification to activate these embeds. Before that, however, the icon doesn't even show up in the urlbar. So, for beta, we need the patch in bug 820497.

(In reply to David Keeler (:keeler) from comment #32) > So, for beta, we need the patch in bug 820497. Er, wait. I'm not sure my reasoning made any sense. I guess long story short is even if we took this patch in beta, there would still be a similar bug where dynamically added plugins of a different type won't show the urlbar icon and thus can't be activated by the popup, so I think we should take that patch too.

Please prepare patches for Aurora/Beta/ESR17 so that we can land before the end of the week. To double verify, it sounds like in FF18 if the user clicks "Activate All Plugins" or whitelists jango.com, they'll be able to play music. Correct?

[Approval Request Comment] Bug caused by (feature/regressing bug #): click-to-play User impact if declined: dynamically-added plugins won't start after a user has activated all of a given type in the popup notification (works fine if they use "Activate All Plugins") Testing completed (on m-c, etc.): on central Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none

[Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch: all same as patch for aurora

[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: needed for click-to-play User impact if declined: see above Fix Landed on Version: 21 Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Again, carrying over reviews for patches due to rebasing for each branch - they have no functional changes. (In reply to Alex Keybl [:akeybl] from comment #34) > To double verify, it sounds like in FF18 if the user clicks "Activate All > Plugins" or whitelists jango.com, they'll be able to play music. Correct? This is more about vk.com at this point, but jango.com works if it is whitelisted on FF18. As per comment 12 and comment 14, jango.com just won't work until they fix their implementation.

(In reply to David Keeler (:keeler) from comment #38) > jango.com works if it is > whitelisted on FF18. Actually only if whitelisted + reload

So, jango.com sounds like an edge case for bug 796039 also.

aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/55120dd921ee beta: https://hg.mozilla.org/releases/mozilla-beta/rev/5932391eb74a esr17: https://hg.mozilla.org/releases/mozilla-esr17/rev/0f66b61edee6

(In reply to Paul Silaghi [QA] from comment #29) > vk.com works fine now on nightly 21.0a1 (2013-01-22) > jango.com doesn't work with CTP enabled, but I guess that's ok based on > comment 12 > Verified fixed on FF 21. Verified fixed FF 19b4 Win7, Ubuntu 12.04, Mac OS X 10.7.5

Verified fixed Aurora 20.0a2 (2013-01-31), 2013-01-30-03-45-01-mozilla-esr17 Win7, Ubuntu 12.04, Mac OS X 10.8.2