Last Comment Bug 554780 - Make plugins provider correctly handle plugins being added and removed through detection at runtime
: Make plugins provider correctly handle plugins being added and removed throug...
Status: RESOLVED FIXED
[rewrite]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla21
Assigned To: Dave Townsend [:mossop]
:
Mentors:
: 675495 682578 724489 843070 (view as bug list)
Depends on: 552799
Blocks: 823085 461973
  Show dependency treegraph
 
Reported: 2010-03-24 15:43 PDT by Dave Townsend [:mossop]
Modified: 2013-03-06 06:56 PST (History)
14 users (show)
ryanvm: in‑testsuite+
hskupin: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
fixed


Attachments
patch rev 1 (16.99 KB, patch)
2013-01-22 14:35 PST, Dave Townsend [:mossop]
blair: review-
Details | Diff | Splinter Review
patch rev 2 (17.48 KB, patch)
2013-02-14 12:24 PST, Dave Townsend [:mossop]
blair: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2010-03-24 15:43:52 PDT
We still need to send events for when new plugins are detected/removed.
Comment 1 [not reading bugmail] 2010-08-19 11:36:25 PDT
Dave, I don't know if you have a bug on this, or this is backend work, although it doesn't look like it, but Remove plugin doesn't appear to do anything.  I was thinking of filing a bug on that.  One plugin that was bothersome is the Bing Bar which gets installed with Windows Live Essentials.  I tried to remove it but it didn't do anything.  I resorted to using the control panel in windows.
Comment 2 Dave Townsend [:mossop] 2010-08-19 11:37:50 PDT
There shouldn't be a remove button for plugins, if there is it is a separate bug that should be filed.
Comment 3 [not reading bugmail] 2010-08-19 11:57:20 PDT
ok, Done.
Comment 4 Tony Mechelynck [:tonymec] 2010-08-20 00:12:05 PDT
(In reply to comment #2)
> There shouldn't be a remove button for plugins, if there is it is a separate
> bug that should be filed.

bug 588888
Comment 5 Dave Townsend [:mossop] 2011-07-31 17:48:14 PDT
*** Bug 675495 has been marked as a duplicate of this bug. ***
Comment 6 Dave Townsend [:mossop] 2011-08-27 11:18:07 PDT
*** Bug 682578 has been marked as a duplicate of this bug. ***
Comment 7 Dave Townsend [:mossop] 2012-02-06 10:36:03 PST
*** Bug 724489 has been marked as a duplicate of this bug. ***
Comment 8 Dave Townsend [:mossop] 2013-01-18 16:23:32 PST
I have a straightforward patch for this. It will make it appear through the add-ons manager API as if updated plugins are simple uninstalled and installed again which is enough to keep things working sane in cases like bug 823085. It still won't be perfect if the UI is open when plugin changes occur and the user attempts to make changes to the old plugin before a re-scan occurs but that seems a rare enough event that we should defer it to a follow-up.
Comment 9 Dave Townsend [:mossop] 2013-01-22 14:35:05 PST
Created attachment 705119 [details] [diff] [review]
patch rev 1
Comment 10 Blair McBride [:Unfocused] (UNAVAILABLE) 2013-02-07 16:46:55 PST
Comment on attachment 705119 [details] [diff] [review]
patch rev 1

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

Just a couple of issues with event order/consistency.

::: toolkit/mozapps/extensions/PluginProvider.jsm
@@ +69,5 @@
>  
>    observe: function(aSubject, aTopic, aData) {
> +    switch (aTopic) {
> +    case AddonManager.OPTIONS_NOTIFICATION_DISPLAYED:
> +      this.getAddonByID(aData, function(plugin) {

Nit: Since you're changing this code anyway, care to name this function?
Also, I wonder if this block of code should be moved into it's own function now.

@@ +237,5 @@
> +      let newWrapper = this.buildWrapper(newList[id]);
> +
> +      if (newWrapper.isActive != oldWrapper.isActive) {
> +        AddonManagerPrivate.callAddonListeners(newWrapper.isActive ? "onEnabling" : "onDisabling", newWrapper, false);
> +        AddonManagerPrivate.callAddonListeners(newWrapper.isActive ? "onEnabled" : "onDisabled", newWrapper);

Shouldn't onEnabled be sent out after updating this.plugins, like onUninstalled is?

Nit: I can haz shorter lines?

@@ +254,5 @@
> +    // Finally send out any new plugins.
> +    for (let plugin of newPlugins) {
> +      AddonManagerPrivate.callInstallListeners("onExternalInstall", null,
> +                                               plugin, null, false);
> +      AddonManagerPrivate.callAddonListeners("onInstalling", plugin, false);

Similarly, onExternalInstall and onInstalling should be sent out before updating this.plugins.

::: toolkit/mozapps/extensions/test/xpcshell/test_pluginchange.js
@@ +215,5 @@
> +
> +// Replacing flash should be detected
> +function run_test_6() {
> +  let oldTag = PLUGINS.splice(0, 1)[0];
> +  let newTag = new PluginTag("Flash 2", "A new crash-free Flash!");

This doesn't seem very realistic ;)
Comment 11 Dave Townsend [:mossop] 2013-02-14 12:06:00 PST
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #10)
> ::: toolkit/mozapps/extensions/test/xpcshell/test_pluginchange.js
> @@ +215,5 @@
> > +
> > +// Replacing flash should be detected
> > +function run_test_6() {
> > +  let oldTag = PLUGINS.splice(0, 1)[0];
> > +  let newTag = new PluginTag("Flash 2", "A new crash-free Flash!");
> 
> This doesn't seem very realistic ;)

Don't destroy my hope!
Comment 12 Dave Townsend [:mossop] 2013-02-14 12:24:53 PST
Created attachment 714035 [details] [diff] [review]
patch rev 2

Updated, would be nice to get this in before the uplift as long as it doesn't break your brain
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-02-15 13:38:59 PST
https://hg.mozilla.org/mozilla-central/rev/4b19fa00a8aa
Comment 15 Georg Fritzsche [:gfritzsche] 2013-02-20 08:07:12 PST
*** Bug 843070 has been marked as a duplicate of this bug. ***
Comment 16 Georg Fritzsche [:gfritzsche] 2013-02-20 08:09:08 PST
Should this possibly be nominated for uplift?
Comment 17 Alex Keybl [:akeybl] 2013-02-27 12:13:41 PST
(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> Should this possibly be nominated for uplift?

Would bug 823085 be the main motivation?
Comment 18 Georg Fritzsche [:gfritzsche] 2013-02-28 02:41:51 PST
(In reply to Alex Keybl [:akeybl] from comment #17)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> > Should this possibly be nominated for uplift?
> 
> Would bug 823085 be the main motivation?

Yes, sorry for the missing details. 
With mainly Flash doing updates now while Firefox is running, the Addon Manager UI can become broken. Unintended changes to plugin states might also be possible.
Comment 19 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-01 14:28:21 PST
Please nominate for uplift, let's get this into our next Beta so it can be tested/verified sooner rather than later.
Comment 20 Dave Townsend [:mossop] 2013-03-01 14:34:18 PST
Comment on attachment 714035 [details] [diff] [review]
patch rev 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Firefox 4
User impact if declined: Add-ons Manager UI displays incorrect information if plugins are updated while Firefox is running
Testing completed (on m-c, etc.): Been in tree for a while
Risk to taking this patch (and alternatives if risky): I think the risk of regression is low, if the code has problems then they should only exhibit themselves in the cases where we were already broken.
String or UUID changes made by this patch: None
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-03-05 06:40:34 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/f4bf8ca3565e
Comment 22 Paul Silaghi, QA [:pauly] 2013-03-06 06:56:57 PST
What exactly needs to be tested here?
While I was trying a Flash update with Firefox opened, I spotted the broken UI from bug 823085.

Note You need to log in before you can comment on or make changes to this bug.