Duplicate plugin dll names cause refresh loop on navigator.plugins.refresh(1)

VERIFIED FIXED

Status

()

VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: serhunt, Assigned: serhunt)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

112 bytes, text/html
Details
6.91 KB, patch
peterlubczynski-bugs
: review+
Details | Diff | Splinter Review
(Assignee)

Description

17 years ago
I am filing this bug mainly not to forget to check this issue on the trunk after  
fix for 119621 goes in. Similar fix for the 0.9.4 branch caused this kind of 
problem and is filed in bugscape report 11998. So, the present bug can be 
reproduced and verified using steps from there. Alternatively, the following 
should do on Windows:

1. make sure you have, say, nppdf32.dll in _both_ Mozilla and 4.x plugins 
folders

2. load a simple html containing navigator.plugins.refresh(1) directive in its 
body (I am going to attach one)

3. see continous reload

Basically, the problem is that our refresh loop prevention mechanism which 
involves calculating the plugins check sum to detect changes before refreshing 
plugins and reloading the page, does not take into account the fact that there 
could be plugin dlls with the same name in different directories.
(Assignee)

Comment 1

17 years ago
Created attachment 68939 [details]
Test case
(Assignee)

Comment 2

17 years ago
Created attachment 69140 [details] [diff] [review]
patch v.1

I added a method |nsPluginHostImpl::IsDuplicatePlugin| which is used to
determine that a plugin in question (when we scan plugin folders) is in fact
the same as we already found in some previous folder and already added to our
plugin list. It is treated as an 'unwanted' plugin in all aspects: it gets
cached in the plugin info list, it is not added to the 'loaded' plugin list,
changes to it are detected during refresh/rescan.

Please review.

Comment 3

17 years ago
Comment on attachment 69140 [details] [diff] [review]
patch v.1

r=peterl
Attachment #69140 - Flags: review+
(Assignee)

Comment 4

17 years ago
|nsPluginHostImpl::IsDuplicatePlugin| might seem confusing as on the first 
glance one may think that on Mac only leaf name are compared, but in fact if 
leaf names are the same it goes further and compares the full pathes too (if 
they are not the same, we already got what we need and no further comparison 
needed), so Mac is fine.
(Assignee)

Comment 5

17 years ago
To clarify the reason for this bug. If we have dup plugins they are not stored 
in our plugin info cache. This causes our change detection mechanism think that 
they have just been added thus resulting in endless refresh loop on 
plugins.refresh(1). I made them added to cache so they now act just like 
'unwanted' plugins and do not scew up change detection code.

Comment 6

17 years ago
Just a quick question, didn't get to really look at the patch very well.  If 
this patch weeds out duplicates, does it take into account version and and 
timestamp diffs?  In other words, is it possible the first plug-in loaded is 
not the latest version and the latest version of the plug-in will get cached as 
an unwanted plugin?

Comment 7

17 years ago
Comment on attachment 69140 [details] [diff] [review]
patch v.1

sr=beard
Attachment #69140 - Flags: superreview+
(Assignee)

Comment 8

17 years ago
Todd: the patch does not change the behaviour in sense of what plugin is going 
to be picked up as a plugin to handle the content. It just fixes the glitch in 
the change detection code. If the version is present in the plugin name field in 
the version stamp the plugin will not be considered duplicate even having the 
same file name. Look at |nsPluginTag::Equals| to see how we compare plugins and 
decide when they are the same. The precedence is still determined by creation 
time (if in the same folder) or directory scan sequence (if in different 
folders).
(Assignee)

Comment 9

17 years ago
Checked in. Marking fixed.

Shrirang, would you please also verify it with the steps from Bugscape 11998?
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 10

17 years ago
sure. 

Comment 11

16 years ago
verified with steps from bugscape bug, things look great.verified on 0812 trunk.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.