Return matching plug-ins in the sorted order (by version number).

Return matching plug-ins in the sorted order (by version number).

In case there are multiple plug-ins in one group (multiple versions
of Flash plug-in for example), returning sorted list that has highest
version first makes sure that Chrome won't try to load outdated
version and end up blocking it in the end.

BUG=13892
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231381

[jam, 2013-10-10 05:04:07]

1) i can't click the side-by-side links?

2) how is this happening, i thought plugin_list_win.cc's IsNewerVersion is
supposed to ensure that only one plugin (the latest) is registered

[Rafał Chłodnicki, 2013-10-10 07:52:44]

On 2013/10/10 05:04:07, jam wrote:
> 1) i can't click the side-by-side links?

Not sure what I can do about that. Maybe submitting review anew would fix it...

> 2) how is this happening, i thought plugin_list_win.cc's IsNewerVersion is
> supposed to ensure that only one plugin (the latest) is registered

I admit I haven't noticed it before but now that I check it, it will only handle
this when both plugins have the same file name (see
PluginList::ShouldLoadPluginUsingPluginList). Flash has different file name for
each version (file name includes version number).

(Before you say that this should be fixed instead... I'm not sure that would be
a good idea. I think it would be risky. For example - I don't remember exactly
how QuickTime plugin set looks like but I believe it had multiple plugins with
same names, handling different mime types).

[asanka, 2013-10-10 15:48:43]

I'll bow out from the reviewers list here since I've only touched this code in
passing.

On 2013/10/10 07:52:44, Rafał Chłodnicki wrote:
> On 2013/10/10 05:04:07, jam wrote:
> > 1) i can't click the side-by-side links?
> 
> Not sure what I can do about that. Maybe submitting review anew would fix
it...

Try uploading the same patch set again.

> > 2) how is this happening, i thought plugin_list_win.cc's IsNewerVersion is
> > supposed to ensure that only one plugin (the latest) is registered
> 
> I admit I haven't noticed it before but now that I check it, it will only
handle
> this when both plugins have the same file name (see
> PluginList::ShouldLoadPluginUsingPluginList). Flash has different file name
for
> each version (file name includes version number).
> 
> (Before you say that this should be fixed instead... I'm not sure that would
be
> a good idea. I think it would be risky. For example - I don't remember exactly
> how QuickTime plugin set looks like but I believe it had multiple plugins with
> same names, handling different mime types).

[jam, 2013-10-11 15:21:38]

On 2013/10/10 07:52:44, Rafał Chłodnicki wrote:
> On 2013/10/10 05:04:07, jam wrote:
> > 1) i can't click the side-by-side links?
> 
> Not sure what I can do about that. Maybe submitting review anew would fix
it...
> 
> > 2) how is this happening, i thought plugin_list_win.cc's IsNewerVersion is
> > supposed to ensure that only one plugin (the latest) is registered
> 
> I admit I haven't noticed it before but now that I check it, it will only
handle
> this when both plugins have the same file name (see
> PluginList::ShouldLoadPluginUsingPluginList). Flash has different file name
for
> each version (file name includes version number).
> 
> (Before you say that this should be fixed instead... I'm not sure that would
be
> a good idea. I think it would be risky. For example - I don't remember exactly
> how QuickTime plugin set looks like but I believe it had multiple plugins with
> same names, handling different mime types).

The intent of the code was that we'd only have entry for each plugin. Flash used
to always be npswf32.dll, I don't know why they started adding version numbers
to the filename.

[Rafał Chłodnicki, 2013-10-11 15:57:33]

On 2013/10/11 15:21:38, jam wrote:
> On 2013/10/10 07:52:44, Rafał Chłodnicki wrote:
> > On 2013/10/10 05:04:07, jam wrote:
> > > 1) i can't click the side-by-side links?
> > 
> > Not sure what I can do about that. Maybe submitting review anew would fix
> it...
> > 
> > > 2) how is this happening, i thought plugin_list_win.cc's IsNewerVersion is
> > > supposed to ensure that only one plugin (the latest) is registered
> > 
> > I admit I haven't noticed it before but now that I check it, it will only
> handle
> > this when both plugins have the same file name (see
> > PluginList::ShouldLoadPluginUsingPluginList). Flash has different file name
> for
> > each version (file name includes version number).
> > 
> > (Before you say that this should be fixed instead... I'm not sure that would
> be
> > a good idea. I think it would be risky. For example - I don't remember
exactly
> > how QuickTime plugin set looks like but I believe it had multiple plugins
with
> > same names, handling different mime types).
> 
> The intent of the code was that we'd only have entry for each plugin. Flash
used
> to always be npswf32.dll, I don't know why they started adding version numbers
> to the filename.

So... what you want me to do?

Like I said, eliminating duplicates based on name itself seems risky. Quicktime
has 5 plugins that differ in filename (quicktime1.dll, quicktime2.dll...) but
all of them have the same name. We can't eliminate those just because they have
the same name.

Also, this fix was triggered by a bug related to the Lightspark plugin which is
an open source Flash replacement. It has different file name and version but
same name (also mime types are similar). We shouldn't treat those as duplicates
and remove the older one because user might want to disable official Flash and
use the alternative.

(Flash wants to update itself when it's being used and locked, that's why the
name changes.)

[jam, 2013-10-14 20:40:39]

On 2013/10/11 15:57:33, Rafał Chłodnicki wrote:
> On 2013/10/11 15:21:38, jam wrote:
> > On 2013/10/10 07:52:44, Rafał Chłodnicki wrote:
> > > On 2013/10/10 05:04:07, jam wrote:
> > > > 1) i can't click the side-by-side links?
> > > 
> > > Not sure what I can do about that. Maybe submitting review anew would fix
> > it...
> > > 
> > > > 2) how is this happening, i thought plugin_list_win.cc's IsNewerVersion
is
> > > > supposed to ensure that only one plugin (the latest) is registered
> > > 
> > > I admit I haven't noticed it before but now that I check it, it will only
> > handle
> > > this when both plugins have the same file name (see
> > > PluginList::ShouldLoadPluginUsingPluginList). Flash has different file
name
> > for
> > > each version (file name includes version number).
> > > 
> > > (Before you say that this should be fixed instead... I'm not sure that
would
> > be
> > > a good idea. I think it would be risky. For example - I don't remember
> exactly
> > > how QuickTime plugin set looks like but I believe it had multiple plugins
> with
> > > same names, handling different mime types).
> > 
> > The intent of the code was that we'd only have entry for each plugin. Flash
> used
> > to always be npswf32.dll, I don't know why they started adding version
numbers
> > to the filename.
> 
> So... what you want me to do?
> 
> Like I said, eliminating duplicates based on name itself seems risky.
Quicktime
> has 5 plugins that differ in filename (quicktime1.dll, quicktime2.dll...) but
> all of them have the same name. We can't eliminate those just because they
have
> the same name.
> 
> Also, this fix was triggered by a bug related to the Lightspark plugin which
is
> an open source Flash replacement. It has different file name and version but
> same name (also mime types are similar). We shouldn't treat those as
duplicates
> and remove the older one because user might want to disable official Flash and
> use the alternative.

Sorry if I wasn't clear, I meant plugin filename. That's what the current code
that does deduplication uses.

> 
> (Flash wants to update itself when it's being used and locked, that's why the
> name changes.)

There are other ways of doing that.. i.e. just renaming the existing one on
disk.

[Rafał Chłodnicki, 2013-10-15 09:31:13]

On 2013/10/14 20:40:39, jam wrote:
> > So... what you want me to do?
> > 
> > Like I said, eliminating duplicates based on name itself seems risky.
> Quicktime
> > has 5 plugins that differ in filename (quicktime1.dll, quicktime2.dll...)
but
> > all of them have the same name. We can't eliminate those just because they
> have
> > the same name.
> > 
> > Also, this fix was triggered by a bug related to the Lightspark plugin which
> is
> > an open source Flash replacement. It has different file name and version but
> > same name (also mime types are similar). We shouldn't treat those as
> duplicates
> > and remove the older one because user might want to disable official Flash
and
> > use the alternative.
> 
> Sorry if I wasn't clear, I meant plugin filename. That's what the current code
> that does deduplication uses.

Now I don't understand what you mean. Even after re-reading the thread.

Given the large turn-around (due to timezone differences mainly) I would
appreciate detailed and precise comments so that I can decide one thing or
another without asking too many additional questions. Otherwise we'll be
ping-ponging here until New Years Eve. :)

So, what do you think is wrong with this fix and how would you like it look like
instead?

[jam, 2013-10-18 16:50:30]

apologies for the delay. feel free to ping me on irc in the future.

lgtm

[commit-bot: I haz the power, 2013-10-19 10:02:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rchlodnicki@opera.com/26541011/16001

[commit-bot: I haz the power, 2013-10-19 11:41:02]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Rafał Chłodnicki, 2013-10-22 16:19:46]

On 2013/10/19 11:41:02, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_rel for step(s) unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

One test exposed a problem with this approach. std::set won't add an element to
the list if the two compared elements are considered equal. As comparator works
on the version, two elements with same version are considered equal (even if
they aren't really).
This approach won't work...

[Rafał Chłodnicki, 2013-10-22 16:35:12]

On 2013/10/22 16:19:46, Rafał Chłodnicki wrote:
> On 2013/10/19 11:41:02, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_rel for step(s) unit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
> 
> One test exposed a problem with this approach. std::set won't add an element
to
> the list if the two compared elements are considered equal. As comparator
works
> on the version, two elements with same version are considered equal (even if
> they aren't really).
> This approach won't work...

Actually std::multiset solves that problem.

[jam, 2013-10-28 16:46:38]

lgtm

[commit-bot: I haz the power, 2013-10-28 16:48:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rchlodnicki@opera.com/26541011/294001

[commit-bot: I haz the power, 2013-10-28 19:44:41]

Change committed as 231381

[dglazkov, 2013-10-28 20:33:36]

I think this change impacted Blink layout tests. I can't tell whether it's a
progression or a regressions. Please take a look and let me know soon:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

The last test is clearly saying FAIL now. Should I roll out?

[dglazkov, 2013-10-28 20:54:13]

On 2013/10/28 20:33:36, Dimitri Glazkov wrote:
> I think this change impacted Blink layout tests. I can't tell whether it's a
> progression or a regressions. Please take a look and let me know soon:
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> 
> The last test is clearly saying FAIL now. Should I roll out?

Reverted in r231401. To help with troubleshooting, the following tests started
failing only on Mac:
fast/css/object-fit-embed.html,plugins/embed-prefers-plugins-for-images.html

[Rafał Chłodnicki, 2014-05-05 17:18:14]

On 2013/10/28 20:54:13, dglazkov wrote:
> On 2013/10/28 20:33:36, Dimitri Glazkov wrote:
> > I think this change impacted Blink layout tests. I can't tell whether it's a
> > progression or a regressions. Please take a look and let me know soon:
> > 
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...
> > 
> > The last test is clearly saying FAIL now. Should I roll out?
> 
> Reverted in r231401. To help with troubleshooting, the following tests started
> failing only on Mac:
> fast/css/object-fit-embed.html,plugins/embed-prefers-plugins-for-images.html

I can only guess that this is due to different order of the plugins. With this
change, tests pick up different plugin for the <embed src="image.png"> tag than
they did before. There are two plugins I know of that handle image/png.
Chromium's/Webkit's test plugin and QuickTime. At least the
http://src.chromium.org/viewvc/blink/trunk/LayoutTests/plugins/embed-prefers-...
test should work with both of them but somehow it has failed with this fix.

So I wonder if there is some other plugin on builders that handles image/png
that the builders don't expect or for example the plugin that is picked up with
this fix crashes (at least one Mac, one builder crashed, according to the test
results).

It's hard to do anything about this without knowing more (for example seeing a
dump of navigator.plugins / navigator.mimeTypes).