554780 - Make plugins provider correctly handle plugins being added and removed through detection at runtime

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Dave Townsend [:mossop]]

We still need to send events for when new plugins are detected/removed.

Comment 1

[[not reading bugmail]]

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]]

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]]

ok, Done.

Comment 4

[Tony Mechelynck [:tonymec]]

(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 8

[Dave Townsend [:mossop]]

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]]

Attachment: patch rev 1

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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]]

(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]]

Attachment: patch rev 2

Updated, would be nice to get this in before the uplift as long as it doesn't break your brain

Comment 13

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b19fa00a8aa

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Georg Fritzsche [:gfritzsche]]

Should this possibly be nominated for uplift?

Comment 17

[Alex Keybl [:akeybl]]

(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]]

(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]

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]]

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]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f4bf8ca3565e

Comment 22

[Paul Silaghi, QA [:pauly]]

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.