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)

RESOLVED FIXED in mozilla22

Status

()

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

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CtPDefault:P1])

Attachments

(1 attachment, 8 obsolete attachments)

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.
David, are the click-to-play per-site settings (always activate etc.) stored per mimetype and site or per plugin tag?
Flags: needinfo?(dkeeler)
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.
Flags: needinfo?(dkeeler)
Thanks. So that means on Flash updates click-to-play per-site settings are lost too.
Blocks: 738698
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).
Assignee: nobody → georg.fritzsche
Priority: -- → P2
Target Milestone: --- → mozilla21
Blocks: 823085

Comment 5

4 years ago
Is it not possible that other plugins also change their names when they change their versions?
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.
Whiteboard: [CtPDefault:P1]
Blocks: 549697
Summary: Flash plugin settings not kept when updating it → 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)
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.
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.
Alright, that sounds much more straight-forward then what i was looking at, should have this up tomorrow.
No longer blocks: 549697
Depends on: 549697
Depends on: 838290
Created attachment 713630 [details] [diff] [review]
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?
Attachment #713630 - Flags: review?(benjamin)
(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.
(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.
Attachment #713630 - Flags: review?(benjamin)
Created attachment 714249 [details] [diff] [review]
Persist plugin state in prefs, v2

https://tbpl.mozilla.org/?tree=Try&rev=e8e5c70e76fc
Attachment #713630 - Attachment is obsolete: true
Attachment #714249 - Flags: review?(benjamin)
Re-pushed try run on non-Win platforms due to -Werror,-Wunused-variable breakage:
https://tbpl.mozilla.org/?tree=Try&rev=71423fb4ee15
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.
Attachment #714249 - Flags: review?(benjamin)
Created attachment 714556 [details] [diff] [review]
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.
Attachment #714249 - Attachment is obsolete: true
Attachment #714556 - Flags: review?(benjamin)
As a bonus that removed one dependency.
Status: NEW → ASSIGNED
No longer depends on: 549697
Created attachment 715417 [details] [diff] [review]
Persist plugin state in prefs, v4

Minor cross-platform test fixup, green on try.
https://tbpl.mozilla.org/?tree=Try&rev=a6d48c438feb
Attachment #714556 - Attachment is obsolete: true
Attachment #714556 - Flags: review?(benjamin)
Attachment #715417 - Flags: review?(benjamin)
Blocks: 840530
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?).
Attachment #715417 - Flags: review?(benjamin) → review-
(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.
Created attachment 718583 [details] [diff] [review]
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.
Attachment #715417 - Attachment is obsolete: true
Attachment #718583 - Flags: review?(benjamin)
Created attachment 719539 [details] [diff] [review]
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
Attachment #718583 - Attachment is obsolete: true
Attachment #718583 - Flags: review?(benjamin)
Attachment #719539 - Flags: review?(benjamin)
Created attachment 722269 [details] [diff] [review]
Persist plugin state in prefs, v7

Minor xpcshell fixup.

https://tbpl.mozilla.org/?tree=Try&rev=f1fd161cb656
Attachment #719539 - Attachment is obsolete: true
Attachment #719539 - Flags: review?(benjamin)
Attachment #722269 - Flags: review?(benjamin)
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?
Attachment #722269 - Flags: review?(benjamin) → review+
Blocks: 852484
Created attachment 726642 [details] [diff] [review]
Persist plugin state in prefs, v8

https://tbpl.mozilla.org/?tree=Try&rev=19efbfc98146
Attachment #722269 - Attachment is obsolete: true
Created attachment 727226 [details] [diff] [review]
Persist plugin state in prefs, v9

Fixed stupid oversight in importing prefs.

https://tbpl.mozilla.org/?tree=Try&rev=87a7de4348be
Attachment #726642 - Attachment is obsolete: true
Green on try and no major changes, pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a553cbe3c31
https://hg.mozilla.org/mozilla-central/rev/4a553cbe3c31
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla21 → mozilla22

Updated

4 years ago
Depends on: 853911
Blocks: 839728
Depends on: 854467
You need to log in before you can comment on or make changes to this bug.