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

()

defect
P2
normal
RESOLVED FIXED
6 years ago
6 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.
Assignee

Comment 1

6 years ago
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)
Assignee

Comment 3

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

Comment 4

6 years ago
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
Assignee

Updated

6 years ago
Blocks: 823085

Comment 5

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

Comment 6

6 years ago
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.

Updated

6 years ago
Whiteboard: [CtPDefault:P1]
Assignee

Updated

6 years ago
Blocks: 549697

Updated

6 years ago
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)
Assignee

Comment 7

6 years ago
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

6 years ago
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.
Assignee

Comment 9

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

Updated

6 years ago
No longer blocks: 549697
Depends on: 549697
Assignee

Updated

6 years ago
Depends on: 838290
Assignee

Comment 10

6 years ago
Posted patch Persist plugin state in prefs (obsolete) — Splinter Review
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)
Assignee

Comment 11

6 years ago
(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.
Assignee

Comment 12

6 years ago
(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.
Assignee

Updated

6 years ago
Attachment #713630 - Flags: review?(benjamin)
Assignee

Comment 14

6 years ago
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)
Assignee

Comment 16

6 years ago
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)
Assignee

Comment 17

6 years ago
As a bonus that removed one dependency.
Status: NEW → ASSIGNED
No longer depends on: 549697
Assignee

Comment 18

6 years ago
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)
Assignee

Updated

6 years ago
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-
Assignee

Comment 20

6 years ago
(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.
Assignee

Comment 21

6 years ago
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)
Assignee

Comment 22

6 years ago
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)
Assignee

Comment 23

6 years ago
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+
Assignee

Updated

6 years ago
Blocks: 852484
Assignee

Comment 26

6 years ago
Fixed stupid oversight in importing prefs.

https://tbpl.mozilla.org/?tree=Try&rev=87a7de4348be
Attachment #726642 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4a553cbe3c31
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla21 → mozilla22

Updated

6 years ago
Depends on: 853911
Assignee

Updated

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