796039 - click-to-play: nsObjectLoadingContent::GetActivated can return true even if a plugin hasn't been activated

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dana Keeler (she/her) [:keeler]]

In browser-plugins.js, activatePlugins activates all plugins depending on what canActivatePlugin says. canActivatePlugin inspects a nsObjectLoadingContent's activated property, which depends on what nsObjectLoadingContent::ShouldPlay says. If the permission is already set to allow, ShouldPlay will return true even if the plugin is not actually activated, and thus the plugin won't get played. So, the permission needs to be set after activating the plugins.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Actually, I'm not sure this is browser-plugin.js's bug. I think the bug is that nsObjectLoadingContent::GetActivated can return true when the plugin has not actually been activated.
John - thoughts on this?

Comment 2

[John Schoenick [:johns]]

You're right that |GetActivated| is currently behaving like a |GetShouldActivate| call, but returning just mActivated is similarly un-helpful for this front-end code: mActivated checks whether or not the plugin was *explicitly* activated, so a plugin can play if mActivated is false but ShouldPlay() is true.

What we really need to do is
> if (fallbackType == eFallbackClickToPlay || fallbackType == eFallbackPlayPreview)
>     && ShouldPlay() && !ShouldPreview()) {
>   PlayPlugin();
> }

So we either need a new |GetIsShowingAlternatePluginContent| call to check for plugin-replacement types (playPreview or CTP) or to just do the checks in the front end.

As a side note, ShouldPlay() is confusing with ShouldPreview() also existing, we should probably rename it ShouldCTP() and have ShouldPlay() check both.

Comment 3

[Dana Keeler (she/her) [:keeler]]

(In reply to John Schoenick [:johns] from comment #2)
> What we really need to do is
> > if (fallbackType == eFallbackClickToPlay || fallbackType == eFallbackPlayPreview)
> >     && ShouldPlay() && !ShouldPreview()) {
> >   PlayPlugin();
> > }

I'm not understanding - where would this go? In GetActivated?

Comment 4

[John Schoenick [:johns]]

(In reply to David Keeler from comment #3)
> I'm not understanding - where would this go? In GetActivated?

Sorry I might not've completely understood the issue here, I was thinking about the case of changing permissions and seeing if a plugin should thus be started.

The issue is activating all plugins that have not yet been activated, correct? |GetActivated| is behaving more like ShouldPlay, which might be false after permissions change.

The only potential problem I see is if we fix GetActivated to return mActivated, then PlayPlugin() on non-activated plugins, we might restart plugins that were already running due to permissions/etc -- mActivated can be false but the plugin may still play if it doesn't require explicit activation.

If that's not an issue with the front-end code (I haven't read over it), then fixing GetActivated to return mActivated is fine with me (assuming other plugin.activated consumers don't depend on this behavior)

Comment 5

[Dana Keeler (she/her) [:keeler]]

Can we make GetActivated return mActivated, and make mActivated an invariant where it is true if and only if the plugin had been loaded? (whether by clicking or because it was allowed to load or whatever?)

Comment 6

[John Schoenick [:johns]]

(In reply to David Keeler from comment #5)
> Can we make GetActivated return mActivated, and make mActivated an invariant
> where it is true if and only if the plugin had been loaded? (whether by
> clicking or because it was allowed to load or whatever?)

The problem is that mActivated lives as long as the object, so just because it has played a plugin before doesn't mean it should be allowed to again. E.g., we load a normal plugin with CTP disabled, then load a plugin that is CTP-by-type. So mActivated tracks whether that object frame has *explicit* user-has-clicked activation. In fact, we might want to look into how that interacts with blocklist-based CTP - if the user has clicked on a normal plugin, they might not be happy with the site then swapping a vulnerable plugin in instead.

So option 1 would be setting mActivated to true whenever non-fallback content passes ShouldPlay() and loads, but we'd need to set it to false when changing types, which would mean every new type would require an explicit user-click.

Otherwise, it depends on what the front end wants to do. We can expose a ShouldPlay IDL (which GetActivated is basically doing now) along with a fixed-GetActivated() and the front end can see as much state as it wants

Comment 7

[Dana Keeler (she/her) [:keeler]]

I think option 1 is the best. This lets us know when a plugin has actually loaded (either because it was allowed to or because the user clicked on it) while still handling the case of a plugin changing type (like you said, if the plugin changes type to a vulnerable/click-to-play one, we don't want it loading just because the previous type of plugin was allowed to load (even if that was also a click-to-play plugin and it was clicked by the user)).
Knowing when a plugin has loaded is important for figuring out when to display the popup notification and so we don't accidentally activate a plugin twice and potentially reset it.

Comment 9

[Dana Keeler (she/her) [:keeler]]

Attachment: workaround for beta/aurora

This was fixed by bug 797043 on trunk, but this bug still affects beta and aurora. Since that patch is unlikely to be uplifted, I'd like to land this workaround.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 745030
User impact if declined: clicking "always activate plugins on this site" will not actually activate any plugins (this is in the click-to-play plugin popup notification)
Testing completed (on m-c, etc.): try run (beta): https://tbpl.mozilla.org/?tree=Try&rev=cb1835ea1bb4
Risk to taking this patch (and alternatives if risky): super low (it switches two lines of js and modifies a test)
String or UUID changes made by this patch: none

Comment 10

[Dana Keeler (she/her) [:keeler]]

(by the way, the failures on the try run appear to happen even without the patch)

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 672826 [details] [diff] [review]
workaround for beta/aurora

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

::: browser/base/content/browser-plugins.js
@@ +508,1 @@
>          Services.perms.add(aBrowser.currentURI, "plugins", Ci.nsIPermissionManager.ALLOW_ACTION);

Yeah, this looks like it will work but we should really fix http://hg.mozilla.org/releases/mozilla-beta/file/06e3c0f40f48/content/base/src/nsObjectLoadingContent.cpp#l2482. I guess that's what this other bug is doing?

Can you include a comment to explain the importance of the ordering here? I hate having to do hacks like this, but I hate them even more when they're not explicit :)

::: browser/base/content/test/browser_pluginnotification.js
@@ +378,5 @@
>    // Simulate clicking the "Allow Always" button.
>    popupNotification.secondaryActions[0].callback();
> +  // Using the "activated" property isn't a foolproof way of determining if
> +  // a plugin has been activated, so see if it's exposing a method we're
> +  // expecting of an activated plugin.

Seems like we should use this approach in other parts of this test too. Can you file a follow-up bug to get this part of the test fixed on trunk?

Comment 12

[Dana Keeler (she/her) [:keeler]]

Attachment: workaround for beta/aurora v2

(In reply to Jared Wein [:jaws] from comment #11)
> Comment on attachment 672826 [details] [diff] [review]
> workaround for beta/aurora
> 
> Review of attachment 672826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-plugins.js
> @@ +508,1 @@
> >          Services.perms.add(aBrowser.currentURI, "plugins", Ci.nsIPermissionManager.ALLOW_ACTION);
> 
> Yeah, this looks like it will work but we should really fix
> http://hg.mozilla.org/releases/mozilla-beta/file/06e3c0f40f48/content/base/
> src/nsObjectLoadingContent.cpp#l2482. I guess that's what this other bug is
> doing?

Yup. I don't think it's likely that fix will be uplifted, and this is a lot safer.

> Can you include a comment to explain the importance of the ordering here? I
> hate having to do hacks like this, but I hate them even more when they're
> not explicit :)

Good idea - added.

> ::: browser/base/content/test/browser_pluginnotification.js
> @@ +378,5 @@
> >    // Simulate clicking the "Allow Always" button.
> >    popupNotification.secondaryActions[0].callback();
> > +  // Using the "activated" property isn't a foolproof way of determining if
> > +  // a plugin has been activated, so see if it's exposing a method we're
> > +  // expecting of an activated plugin.
> 
> Seems like we should use this approach in other parts of this test too. Can
> you file a follow-up bug to get this part of the test fixed on trunk?

I wasn't clear with this comment - it's not foolproof as long as bug 797043 isn't fixed, but when it is, it should be fine. Although maybe we could just add this as another test to hopefully catch any regressions of this sort.

Anyway, carrying over r+.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 673026 [details] [diff] [review]
workaround for beta/aurora v2

Adding qawanted and verifyme to get some QA attention around this bug specifically.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Let's get this landed on trunk and branches by end of day tomorrow please so that we can qa & collect feedback on Beta 3.

Comment 15

[Dana Keeler (she/her) [:keeler]]

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/1ff41bb6a63a
Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/c6ec0dc72b64

Comment 16

[Dana Keeler (she/her) [:keeler]]

So, this was fixed in trunk (19) in bug 797043 - I don't know how to reflect that in the tracking/status flags.

Comment 17

[Kevin Brosnan [Ex-Mozilla]]

I think this is about as close to what makes sense.

Comment 18

[u279076]

Are there reliable steps to reproduce this bug which we can use in verification of the fix?

Comment 19

[Dana Keeler (she/her) [:keeler]]

IIRC, if you clicked "always activate plugins for this site" (in the popup notification in the drop-down button), plugins wouldn't actually activate until you refreshed the page (whereupon they would activate because the permission had been set). They should activate when you click the button.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #18)
> Are there reliable steps to reproduce this bug which we can use in
> verification of the fix?

From Bug 801480:
>1) Turn on "click to play" plugins ("plugins.click_to_play" = true in about:config)
>2) Go to http://www.youtube.com/watch?v=f_iQRO5BdCM (or other page with plugins)
>3) Click the puzzle piece to open plugins menu
>4) Select the drop-down menu for more options
>5) Select "Always active plugins for this site"
>
>Expected: The preference is set to always active plugins and the plugins are activated at this point (similar to how selecting the first option, "Activate all plugins" will active plugins without refreshing the page
>Actual: The preference is set but the plugins are not activated until they are clicked or the page is reloaded.

Comment 21

[u279076]

Thanks Michael, I'll coordinate some testing around that test case.

Comment 22

[Paul Silaghi, QA [:pauly]]

"Always activate plugins for this site" activate the plugins automatically without refresh.
Verified fixed on FF 17b5, 18.0a2(2012-11-06) and 19.0a1(2012-11-06) on Win 7, Mac OS X 10.6.8 and Ubuntu 12.04