830267 - Don't store plugin preferences via pluginreg.dat: store them per-mimetype (Flash plugin settings not kept when updating it)(Java plugin reenabled randomly)(All plugins reenabled when switching architectures on mac)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Georg Fritzsche [:gfritzsche]]

When Flash puts a new DLL in place during update it uses a different file name, hence we pick it up as a new/different plugin and never carry over any of the properties of the plugin tag.

While this shouldn't be an issue with other plugins, we might want to look into at least carrying those over to newly found Flash plugins.

Comment 1

[Georg Fritzsche [:gfritzsche]]

David, are the click-to-play per-site settings (always activate etc.) stored per mimetype and site or per plugin tag?

Comment 2

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

They're per filename: https://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#1186 (and then stored in the permission manager per site)
That mechanism could probably be improved.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Thanks. So that means on Flash updates click-to-play per-site settings are lost too.

Comment 4

[Benjamin Smedberg]

FWIW, this is a hard problem because the set of MIME types, the description, and the filename can all theoretically change between plugin version. Since the MIME types are the least likely to change, we should probably do something like this:

* save the preferences based on MIME types
* when loading a plugin, check it against all the saved MIME types and apply the most restrictive preference to the entire plugin (so if a plugin adds a new MIME type as Java does for its major releases, it will continue to either be disabled or CTP)
* do not save these settings in pluginreg.dat. Either save them in prefs or in addon manager metadata.

gfritzsche, can you please implement this for FF21 (after any of the things we need for the UR experiment).

Comment 5

[David E. Ross]

Is it not possible that other plugins also change their names when they change their versions?

Comment 6

[Benjamin Smedberg]

Possible yes, likely no. Note that this is only really a problem if the plugin updates *while Firefox is running*. That's pretty unusual and we had to fix issues in order to get it working for Flash.

Comment 7

[Georg Fritzsche [:gfritzsche]]

The possible options i see here (see also bug 838290, comment 1) would be:

(1) for each mimetype listed by plugin, store {mimetype,enabledState}
(2) store (mimetypeSetOfPlugin,enabledState)
(3) fix-up discovery of common plugins (Flash & Java)

(3) would be just a fallback, doing the same thing as before and only fix known plugin where this is a confirmed issue.

(1) has problems when a mimetype is supported by more than one plugin (image/video/pdf comes to mind). Applying the most restrictive state for any mimetype a plugin supports means we couldn't apply less restrictive permissions on only of them.
This could be softened up by only looking those up when a new plugin file is found and otherwise using the state stored per plugin-file.

(2) obviously doesn't work when the set of supported mimetypes changes (e.g. Java), but we could base this on the best subset-match.

Comment 8

[Benjamin Smedberg]

Bug 838290 has a "decision", and I think we can follow that same pattern for this bug, and just store the enabled state in prefs. ("plugin.<name>.state")

I think we should attempt to import the pluginreg.dat prefs on upgrade.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Alright, that sounds much more straight-forward then what i was looking at, should have this up tomorrow.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs

Test will follow.

Noteworthy:

I switched to pref-name "plugin.state.<niceName>" to get some resemblance of order in the prefs.

I'm not sure if we, by importing the click-to-play flag, involuntarily complicate turning on CtPDefault. What would we use for that and would this block it?

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> Created attachment 713630 [details] [diff] [review]

Also: this is based on bug 838290 and bug 549697.

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> I'm not sure if we, by importing the click-to-play flag, involuntarily
> complicate turning on CtPDefault.

Nevermind, we obviously don't need to import the CtP-flag as there is no UI yet to set it.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v2

https://tbpl.mozilla.org/?tree=Try&rev=e8e5c70e76fc

Comment 14

[Georg Fritzsche [:gfritzsche]]

Re-pushed try run on non-Win platforms due to -Werror,-Wunused-variable breakage:
https://tbpl.mozilla.org/?tree=Try&rev=71423fb4ee15

Comment 15

[Benjamin Smedberg]

Comment on attachment 714249 [details] [diff] [review]
Persist plugin state in prefs, v2

as noted on IRC, this patch actually implements policy changes making plugins ctp by default, and we don't want to make those changes yet.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v3

Does that look better?
I pulled out most CtP changes, but left it so that it's easy to re-add later.

Comment 17

[Georg Fritzsche [:gfritzsche]]

As a bonus that removed one dependency.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v4

Minor cross-platform test fixup, green on try.
https://tbpl.mozilla.org/?tree=Try&rev=a6d48c438feb

Comment 19

[Benjamin Smedberg]

Comment on attachment 715417 [details] [diff] [review]
Persist plugin state in prefs, v4

nsPluginHost::LoadStateForTag is a confusing function. It seems that the caller is nsPluginHost::ScanPluginsDirectory. Is that the only function which can create a plugin tag? Why are we loading the state from an existing tag? Wouldn't the pref contain the same information?

What is the interaction between the plugin state and the plugin flags? Perhaps we ought to get rid of Mark/UnMark/SetFlag/Flags entirely and replace that with specific query functions? It seems a little silly to be storing the enabled state of a plugin in both the flags and prefs; could we avoid storing the enabled state in the tag at all and only do it in prefs?

Why is nsPluginHost::SaveStateForTag on nsPluginHost at all? I think that function could be entirely on nsPluginTags.

Why is the pref plugin.state.* instead of plugin.enabled.* ? What other state (other than the enabled/disabled/CtP state) are you thinking of storing in this pref?

nsPluginHost::PersistStateForTag really looks like it should be "ImportStateForTag". But I'm not sure I understand why we'd run this at every startup (instead of just once and then set a pref). Also it seems to set prefs directly... why couldn't it just call SaveStateForTag?

Overall, there needs to be more doccomments on the functions explaining what they do (and don't do). As in, I had to follow the code really carefully to figure out that when the extension manager calls nsPluginTag::SetDisabled, we called Mark(pluginflagenabled) which calls SaveStateForTag which is what actually sets the pref. And I'm still not completely clear on what `triggerNotification` is for (disabling UpdatePluginInfo, but when wouldn't we want to call that?).

Comment 20

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> Why is the pref plugin.state.* instead of plugin.enabled.* ? What other
> state (other than the enabled/disabled/CtP state) are you thinking of
> storing in this pref?

Currently we have (independent) flags for both enabled & click-to-play. 
We could probably go for either
* plugin.state.* storing the flags
* plugin.state.* mapping between the flags and enabled/ctp/disabled
* storing the flags as plugin.enabled.*, plugin.clicktoplay.*
* dropping the flags in favour of a enabled/ctp/disabled state on the tag, saved as plugin.state.*
The last one is probably preferrable, but might require some more changes.

Going through the rest later when it's not late evening.

Comment 21

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v5

How does this look?

This:
* strips out the flags completely and persists the plugin states enabled, clicktoplay and blocklisted in the prefs
* is much simpler now
* moves the functionality mostly to nsPluginTag
* renames what was called nsPluginTag::IsEnabled() as it was inconsistent with nsPluginTag::*Disabled()

Also still fixes potential casing issues with the NiceFileName and cleans up the tests a bit.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v6

Fixed two issues uncovered on try:
* need to bump the plugin registry version number so we can tell when we never need to import from it
* "plugin-info-updated" notification triggers were missing

https://tbpl.mozilla.org/?tree=Try&rev=b2149d33411b

Comment 23

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v7

Minor xpcshell fixup.

https://tbpl.mozilla.org/?tree=Try&rev=f1fd161cb656

Comment 24

[Benjamin Smedberg]

Comment on attachment 722269 [details] [diff] [review]
Persist plugin state in prefs, v7

>diff --git a/dom/plugins/base/nsPluginHost.cpp b/dom/plugins/base/nsPluginHost.cpp

>+  // we need to import the legacy flags from the plugin registry once
>+  const char pluginStateImportedPref[] = "plugin.importedState";
>+  const bool pluginStateImported =
>+    Preferences::GetDefaultBool(pluginStateImportedPref, false);

I think the extra variable for the prefname is probably unnecesary, just do

Preferences::GetDefaultBool("plugin.importedState", false) ?

>diff --git a/dom/plugins/base/nsPluginTags.cpp b/dom/plugins/base/nsPluginTags.cpp

I think we should not have separate "enabled" and "clicktoplay", instead have a single "enableState" or even just "state" with integer values, e.g. 0 == disabled 1 == click-to-play 2 == enabled

r=me with that change. I'm happy to re-review if you're not sure about it.

Also as noted, I'm leery about using prefs as a cache for the "blocklisted" data. I'd really prefer if the blocklist service does its own cacheing if necessary. But I don't want to hold up this patch on that behavior, so please file a followup and coordinate with Mossop/Unfocused to get that less weird later?

Comment 25

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v8

https://tbpl.mozilla.org/?tree=Try&rev=19efbfc98146

Comment 26

[Georg Fritzsche [:gfritzsche]]

Attachment: Persist plugin state in prefs, v9

Fixed stupid oversight in importing prefs.

https://tbpl.mozilla.org/?tree=Try&rev=87a7de4348be

Comment 27

[Georg Fritzsche [:gfritzsche]]

Green on try and no major changes, pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a553cbe3c31

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a553cbe3c31