can't un-set the clicktoplay flag on an nsIPluginTag

RESOLVED FIXED in mozilla23

Status

()

Core
Plug-ins
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

({addon-compat})

Trunk
mozilla23
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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);
}
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.
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 }
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.
Created attachment 729122 [details] [diff] [review]
temporary patch
(Assignee)

Updated

4 years ago
Blocks: 549697
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.
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 :(
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
Attachment #730845 - Flags: feedback?(georg.fritzsche)
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|.
Attachment #730845 - Flags: feedback?(georg.fritzsche) → feedback+
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.
Assignee: nobody → dkeeler
Attachment #729122 - Attachment is obsolete: true
Attachment #730845 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #731953 - Flags: review?(benjamin)
Yep, the tests look pretty good: https://tbpl.mozilla.org/?tree=Try&rev=2b578ca55b97
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.
Attachment #731953 - Flags: review?(benjamin) → review+
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.
Attachment #731953 - Attachment is obsolete: true
Attachment #734723 - Flags: review?(benjamin)
Attachment #734723 - Flags: review?(benjamin) → review+
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!
Attachment #734723 - Flags: review?(bmcbride)
Comment on attachment 734723 [details] [diff] [review]
patch v2

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

Me like.
Attachment #734723 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9983874bfa63
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
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
https://hg.mozilla.org/mozilla-central/rev/e30393f6f85b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

4 years ago
Depends on: 866557
Keywords: addon-compat

Comment 19

4 years ago
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
Using the "enabledState" attribute with Ci.nsIPluginTag.STATE_CLICKTOPLAY / STATE_ENABLED / STATE_DISABLED should do what you want.

Comment 21

4 years ago
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
Depends on: 916026
You need to log in before you can comment on or make changes to this bug.