Last Comment Bug 854467 - can't un-set the clicktoplay flag on an nsIPluginTag
: can't un-set the clicktoplay flag on an nsIPluginTag
Status: RESOLVED FIXED
: addon-compat
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: David Keeler [:keeler] (use needinfo?)
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on: 866557 916026
Blocks: 549697 830267
  Show dependency treegraph
 
Reported: 2013-03-25 10:16 PDT by David Keeler [:keeler] (use needinfo?)
Modified: 2013-09-13 01:46 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
temporary patch (3.25 KB, patch)
2013-03-25 12:07 PDT, David Keeler [:keeler] (use needinfo?)
no flags Details | Diff | Splinter Review
patch for new api (25.95 KB, patch)
2013-03-28 13:24 PDT, David Keeler [:keeler] (use needinfo?)
gfritzsche: feedback+
Details | Diff | Splinter Review
patch (26.24 KB, patch)
2013-04-01 11:16 PDT, David Keeler [:keeler] (use needinfo?)
benjamin: review+
Details | Diff | Splinter Review
patch v2 (27.12 KB, patch)
2013-04-08 11:06 PDT, David Keeler [:keeler] (use needinfo?)
benjamin: review+
blair: review+
Details | Diff | Splinter Review

Description David Keeler [:keeler] (use needinfo?) 2013-03-25 10:16:50 PDT
Basically, this fails (run as an xpcshell-test in dom/plugins/test/unit/):

function run_test() {
  let tag = get_test_plugintag();
  tag.clicktoplay = true;
  do_check_true(tag.clicktoplay);
  tag.clicktoplay = false;
  do_check_false(tag.clicktoplay);
}
Comment 1 Benjamin Smedberg [:bsmedberg] 2013-03-25 11:20:08 PDT
Which check fails? There are only three states for a plugin, currently:

* enabled
* clicktoplay
* disabled

So if a plugin is in the disabled state, [get] .clicktoplay will always be false, and [set] .clicktoplay will have no effect. We did this because we wanted to implement bug 830267 without doing external API changes; we can certainly make the API reflect reality a little better in the future.
Comment 2 David Keeler [:keeler] (use needinfo?) 2013-03-25 11:25:29 PDT
The second check fails. Basically, the implementation for [set] .clicktoplay can never unset the click-to-play state. Unfortunately, it's not even clear what that should do.

nsPluginTags.cpp:

409 NS_IMETHODIMP
410 nsPluginTag::SetClicktoplay(bool clicktoplay)
411 {
412   if (clicktoplay == IsClicktoplay()) {
413     return NS_OK;
414   }
415 
416   const PluginState state = GetPluginState();
417   if (state != ePluginState_Disabled) {
418     SetPluginState(ePluginState_Clicktoplay);
419   }
420 
421   mPluginHost->UpdatePluginInfo(this);
422   return NS_OK;
423 }
Comment 3 David Keeler [:keeler] (use needinfo?) 2013-03-25 11:31:27 PDT
I guess the real question is, "If pluginTag.clicktoplay is true, what do we do to put that plugin in a state where disabled is false and clicktoplay is also false?"
Setting clicktoplay to false won't do it, because that doesn't do anything. Setting disabled to false won't do it either, because SetDisabled(false) calls SetEnabled(true), which exits early without changing any state if IsEnabled() is true, which is the case if the plugin's state is ePluginState_Clicktoplay.
Comment 4 David Keeler [:keeler] (use needinfo?) 2013-03-25 12:07:50 PDT
Created attachment 729122 [details] [diff] [review]
temporary patch
Comment 5 Benjamin Smedberg [:bsmedberg] 2013-03-26 10:12:06 PDT
So I think the "right" way to fix this is as follows:

readonly attribute boolean disabled;
readonly attribute boolean clicktoplay;

const unsigned long STATE_ENABLED = 0;
const unsigned long STATE_DISABLED = 1;
const unsigned long STATE_CLICKTOPLAY = 2;

attribute unsigned long enabledState;

But if that's going to be too complicated for your dependent patches, we should take a workaround and fix the API in a subsequent step.
Comment 6 Georg Fritzsche [:gfritzsche] 2013-03-28 08:40:42 PDT
I can probably look into the proper API changes next week.

I don't see a problem with a temporary patch if bug 549697 needs to land before that is done - currently the changes in 549697 are the only ones that actually need this. I actually intended to cover that API usage, but apparently didn't quite see it through :(
Comment 7 David Keeler [:keeler] (use needinfo?) 2013-03-28 13:24:38 PDT
Created attachment 730845 [details] [diff] [review]
patch for new api

I was concerned about how many tests would break if we made these changes, so I actually slapped together something yesterday and ran it on try. There were a few bugs, so I fixed it up and now I'm running it again: https://tbpl.mozilla.org/?tree=Try&rev=125376b6d29b
Comment 8 Georg Fritzsche [:gfritzsche] 2013-03-30 01:36:49 PDT
Comment on attachment 730845 [details] [diff] [review]
patch for new api

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

Apart from the missing notification trigger this looks good to me.
Thanks for checking into this.

::: dom/plugins/base/nsPluginTags.cpp
@@ +385,5 @@
> +NS_IMETHODIMP
> +nsPluginTag::SetEnabledState(uint32_t aEnabledState) {
> +  if (aEnabledState >= ePluginState_MaxValue)
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  Preferences::SetInt(GetStatePrefNameForPlugin(this).get(), aEnabledState);

This is now missing the |host->UpdatePluginInfo(this)| for |oldState != newState|.
Comment 9 David Keeler [:keeler] (use needinfo?) 2013-04-01 11:16:32 PDT
Created attachment 731953 [details] [diff] [review]
patch

Thanks, Georg. This is actually passing (almost?) all of the tests now, so I think we can go ahead with making these changes.
Comment 10 David Keeler [:keeler] (use needinfo?) 2013-04-01 15:21:30 PDT
Yep, the tests look pretty good: https://tbpl.mozilla.org/?tree=Try&rev=2b578ca55b97
Comment 11 Benjamin Smedberg [:bsmedberg] 2013-04-08 08:34:29 PDT
Comment on attachment 731953 [details] [diff] [review]
patch

Please add static-asserts that each of nsIPluginTag.STATE_* == nsPluginTag.ePluginState_*

And also add a comment to nsIPluginTag.idl that the numeric values are stored in prefs, so if new states are added, they should not renumber the existing states.
Comment 12 David Keeler [:keeler] (use needinfo?) 2013-04-08 11:06:48 PDT
Created attachment 734723 [details] [diff] [review]
patch v2

Thanks for the review - I would appreciate one more quick look just to make sure I put the static asserts in the appropriate place.
Comment 13 David Keeler [:keeler] (use needinfo?) 2013-04-08 11:29:37 PDT
Comment on attachment 734723 [details] [diff] [review]
patch v2

It would probably also be a good idea for Blair to have a look at these changes. Thanks!
Comment 14 Blair McBride [:Unfocused] (UNAVAILABLE) 2013-04-08 20:18:52 PDT
Comment on attachment 734723 [details] [diff] [review]
patch v2

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

Me like.
Comment 15 David Keeler [:keeler] (use needinfo?) 2013-04-09 09:40:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9983874bfa63
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-04-09 10:48:57 PDT
Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/974726290de1

https://tbpl.mozilla.org/php/getParsedLog.php?id=21601515&tree=Mozilla-Inbound

cc1plus: warnings being treated as errors
../../../../dom/plugins/base/nsPluginTags.cpp: In member function 'void nsPluginTag::SetPluginState(nsPluginTag::PluginState)':
../../../../dom/plugins/base/nsPluginTags.cpp:411:70: error: comparison between 'enum nsPluginTag::PluginState' and 'enum nsIPluginTag::<anonymous>'
../../../../dom/plugins/base/nsPluginTags.cpp:412:73: error: comparison between 'enum nsPluginTag::PluginState' and 'enum nsIPluginTag::<anonymous>'
../../../../dom/plugins/base/nsPluginTags.cpp:413:69: error: comparison between 'enum nsPluginTag::PluginState' and 'enum nsIPluginTag::<anonymous>'
make[6]: *** [nsPluginTags.o] Error 1
Comment 17 David Keeler [:keeler] (use needinfo?) 2013-04-10 10:33:57 PDT
Ok - fixed that and enabled warnings-as-errors on my local build (so hopefully this sort of thing won't happen again).

https://hg.mozilla.org/integration/mozilla-inbound/rev/e30393f6f85b
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-04-11 12:45:26 PDT
https://hg.mozilla.org/mozilla-central/rev/e30393f6f85b
Comment 19 Alexandre Alapetite 2013-07-09 15:38:47 PDT
Hello,
Sorry to revive this bug, but it seems that changeset 128189 by David Keeler to address this bug 854467 has modified the nsIPluginTag Interface to limit the "enabled" attribute to be readonly in dom/plugins/base/nsIPluginTag.idl:
https://hg.mozilla.org/mozilla-central/rev/9983874bfa63#l3.27

As a consequence, some plug-in management extension are broken in Firefox 23 beta (work in Firefox 22) such as Plugins Toggler 1.2.2 https://addons.mozilla.org/firefox/addon/plugins-toggler/

Is the addition of "readonly" to "nsIPluginTag.enabled" really on purpose and needed?

And if yes, what is the alternative approach for e.g. the above-cited extension to work?

Best regards,
Alexandre
http://alexandre.alapetite.fr
Comment 20 David Keeler [:keeler] (use needinfo?) 2013-07-09 15:44:41 PDT
Using the "enabledState" attribute with Ci.nsIPluginTag.STATE_CLICKTOPLAY / STATE_ENABLED / STATE_DISABLED should do what you want.
Comment 21 Alexandre Alapetite 2013-07-09 16:49:58 PDT
Thanks David for this fast answer.

Problem solved from my side, but I hope not too many extensions are broken.

If anyone lands here for the same issue, the relevant information is in:
https://hg.mozilla.org/mozilla-central/file/9983874bfa63/dom/plugins/base/nsIPluginTag.idl

Note You need to log in before you can comment on or make changes to this bug.