The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 20

Status

()

Toolkit
Add-ons Manager
P2
normal
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(firefox19 wontfix, firefox20+ fixed, firefox21 fixed)

Details

(Whiteboard: [rewrite])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
We still need to send events for when new plugins are detected/removed.
Flags: in-testsuite?
Flags: in-litmus-
(Assignee)

Updated

7 years ago
Assignee: dtownsend → nobody
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.
(Assignee)

Comment 2

7 years ago
There shouldn't be a remove button for plugins, if there is it is a separate bug that should be filed.
ok, Done.
(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
(Assignee)

Updated

6 years ago
Duplicate of this bug: 675495
(Assignee)

Updated

6 years ago
Duplicate of this bug: 682578
(Assignee)

Updated

5 years ago
Duplicate of this bug: 724489
(Assignee)

Updated

5 years ago
Summary: Implement additional events for plugins provider → Make plugins provider correctly handle plugins being added and removed through detection at runtime
(Assignee)

Updated

4 years ago
Blocks: 823085
(Assignee)

Comment 8

4 years ago
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.
Assignee: nobody → dtownsend+bugmail
(Assignee)

Comment 9

4 years ago
Created attachment 705119 [details] [diff] [review]
patch rev 1
Attachment #705119 - Flags: review?(bmcbride)
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 ;)
Attachment #705119 - Flags: review?(bmcbride) → review-
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
(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!
(Assignee)

Comment 12

4 years ago
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
Attachment #705119 - Attachment is obsolete: true
Attachment #714035 - Flags: review?(bmcbride)
Attachment #714035 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b19fa00a8aa
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/4b19fa00a8aa
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Duplicate of this bug: 843070
Should this possibly be nominated for uplift?
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → fixed
tracking-firefox20: --- → ?
(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> Should this possibly be nominated for uplift?

Would bug 823085 be the main motivation?
(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.
Please nominate for uplift, let's get this into our next Beta so it can be tested/verified sooner rather than later.
tracking-firefox20: ? → +
(Assignee)

Comment 20

4 years ago
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
Attachment #714035 - Flags: approval-mozilla-beta?
Attachment #714035 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/f4bf8ca3565e
status-firefox19: affected → wontfix
status-firefox20: affected → fixed
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.
You need to log in before you can comment on or make changes to this bug.