Closed Bug 1131136 Opened 5 years ago Closed 5 years ago

Reload plugins in more cases to pick up newly installed versions of Flash after updates

Categories

(Core :: Plug-ins, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(2 files)

When Flash is installed (or auto-updated), Firefox may not notice that a new version is available immediately. We should do the following things in the client:

* re-scan for plugins when we load the plugins pane of the addon manager
* watch for changes to the registry and re-scan on changes
When Silent AutoUpdate for Flash was first written, it used to call navigator.plugins.refresh(false) to refresh the list of plugins. I believe it still does this today. The reason why this had seemingly no effect is described in bug 686335. Although patches landed, they were later backed out. The bug was left as Resolved/Fixed. Maybe it's sufficient to respond to navigator.plugins.refresh(false) by updating the entry for Flash?
After thinking about this some more, I recall a few more details that I thought were worth mentioning: The |navigators.plugins.refresh(false)| call was initiated during NPP_New when Flash detected that a newer version was available on disk (after an auto-update, for example). The expectation was that |navigators.plugins.refresh(false)| would refresh the list of plugins and any subsequent instances of Flash would be using the new version.
Attached patch plugin-regkeySplinter Review
Stephen, I think we fixed the issues with selecting the wrong Flash plugin in various bugs along the way: we started by using file timestamps but that did not work well, and we ended up comparing version numbers in bug 686335: http://hg.mozilla.org/mozilla-central/rev/7c92091ea0b8
Attachment #8561610 - Flags: review?(jmathies)
Attachment #8561610 - Flags: feedback?(spohl.mozilla.bugs)
Blair, what would the right way be to call navigator.plugins.refresh() when the user opens the plugins pane in the addons manager?
Flags: needinfo?(bmcbride)
Comment on attachment 8561610 [details] [diff] [review]
plugin-regkey

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

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Stephen, I think we fixed the issues with selecting the wrong Flash plugin
> in various bugs along the way: we started by using file timestamps but that
> did not work well, and we ended up comparing version numbers in bug 686335:
> http://hg.mozilla.org/mozilla-central/rev/7c92091ea0b8

I may be getting confused by the numerous comments in bug 686335, but my understanding was that the patch there was ultimately backed out again (bug 686335 comment 85). The tracking flags also seem to confirm that. In general, I think it's great to watch the registry key for changes as this should keep working even if Flash's updater stops calling navigator.plugins.refresh() for some reason. My concern however is that (if bug 686335 didn't get fixed) watching the registry key will do no good, since we will run into the same limitation that caused navigator.plugins.refrsh() to fail in the first place (i.e. we will use the first (old) plugin matching the mime type and ignore any subsequent ones, which means we'll be ignoring the more recent plugin).
Attachment #8561610 - Flags: feedback?(spohl.mozilla.bugs)
Hmm, interestingly enough, the reversal of scan order that I suggested in bug 686335 comment 18 ultimately made it into the tree via bug 776923. So, although bug 686335 may have been backed out, this may have been fixed via bug 776923? If that's the case, that'd obviously be great and you can ignore my concerns previously raised here! :-)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Blair, what would the right way be to call navigator.plugins.refresh() when
> the user opens the plugins pane in the addons manager?

Can just do it from the UI (nagivator.plugins is accessible there), or expose nsPluginHost::ReloadPlugins() in nsIPluginHost.idl (we should probably have that anyway).

Though, the patch already calls ReloadPlugins(). If the UI isn't picking up on any changes from that, nsPluginHost may not be sending the "plugins-list-updated" notification.
Flags: needinfo?(bmcbride)
Yeah, I mean "where is an appropriate spot in the UI code"? I looked through extensions.js and didn't see a hook for when the plugins page is opened.

The point of the addon manager change is mostly for the non-Windows case.
Flags: needinfo?(bmcbride)
Stephen, I don't know all about the backout history, but the code that compares versions is in the current trunk version: http://hg.mozilla.org/mozilla-central/annotate/3436787a82d0/dom/plugins/base/nsPluginHost.cpp#l1171
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Yeah, I mean "where is an appropriate spot in the UI code"? I looked through
> extensions.js and didn't see a hook for when the plugins page is opened.
> 
> The point of the addon manager change is mostly for the non-Windows case.

Oh!

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2589

In gListView.show(), when aType=="plugin"
Flags: needinfo?(bmcbride)
Comment on attachment 8561610 [details] [diff] [review]
plugin-regkey

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

::: dom/plugins/base/nsPluginHost.cpp
@@ +2116,5 @@
>    mPluginEpoch = aEpoch;
>  }
>  
> +static void
> +WatchRegKey(uint32_t aRoot, nsCOMPtr<nsIWindowsRegKey>& key)

nit - aKey

also, maybe assert here if aKey is valid or just return early.
Attachment #8561610 - Flags: review?(jmathies) → review+
Attached patch 1131136-partBSplinter Review
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #8562190 - Flags: review?(bmcbride)
Comment on attachment 8562190 [details] [diff] [review]
1131136-partB

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

Can't think of any reasonable way to test this :\
Attachment #8562190 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/708a10a8ac10
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f06821072ef

I added a not-null-check instead of an assert at the beginning of WatchRegKey, because there are tests which purposefully initialize nsPluginHost multiple times and were hitting the assert.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/708a10a8ac10
https://hg.mozilla.org/mozilla-central/rev/6f06821072ef
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1307216
You need to log in before you can comment on or make changes to this bug.