Closed Bug 854467 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Keywords: addon-compat)

Attachments

(1 file, 3 obsolete files)

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.
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 :(
Attached patch patch for new api (obsolete) — Splinter Review
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+
Attached patch patch (obsolete) — Splinter Review
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)
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+
Attached patch patch v2Splinter Review
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+
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
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 866557
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.
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.