Plug-ins refreshed on page on every other refresh interval

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
Plug-ins
--
blocker
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: TaylorToddK, Assigned: av (gone))

Tracking

Trunk
mozilla0.9.9
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
Performing javascript:navigator.plugins.refresh calls from inside a plug-in 
only work with every other attempt, javascript reporting Code: 1013, Module: 
14, Severity: 1, NS_ERROR_DOM_RETVAL_UNDEFINED.

Easiest way to repro this is go to a page that will display the default plug-in 
puzzle piece.  Then, call javascript:navigator.plugins.refresh in the URLbar.  
You can visibly see the default handler being re-loaded only every other time.
Was able to reproduce this with several browsers from the 0.9.4, including NS 
6.2.1.

Comment 1

16 years ago
Created attachment 64649 [details]
testcase

Yes, I see this only working every other time on the trunk too. I verified that
xpti (and I'm sure other things) are only called to update on every other call.
I attached a trivial test case. Whatever errors might be being thrown are eaten
(somewhere) rather than showing up in the JS console for this test case where
the refresh is triggered from a button's onclick handler.

Comment 2

16 years ago
It is getting tripped up by the code that tries to prevent recursive reload at
the top of PluginArrayImpl::Refresh...
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsPluginArray.cpp#173

I'd think that something would be clearing this sentinel when a legitimate
reload finishes - and is past the point of participating in an unexpected
recursion. *Perhaps* at the end of PluginArrayImpl::Refresh (after calling
'Refresh' and before returning?). I don't know this code anywhere near well
enough to say for sure.

This is certainly not good.


Comment 3

16 years ago
Sounds like bug 119621, a regression from bug 93351.

Is there a better way we can check for the recursive page loading condition? Is
there already such a mechanism somehwere else? Maybe we can use the document
referrer or session history? 

I like Brian Nesse's descriptiong of how 4.x did this:
http://bugzilla.mozilla.org/show_bug.cgi?id=93351#c11
Maybe we could have plugin code return failure from ReloadPlugins(1) if it
detected no plugins where changed and then not reload the current page?

Comment 4

16 years ago
Created attachment 64674 [details] [diff] [review]
patch for trunk to use plugin time stamp as checksum

I need to go for now, but here's a possible patch to use the plugin's timestamp
that we get from registry caching as a checksum to see if any plugins changed
during refresh. In some simple testing, it stopped recursion and the button in
this testcase did reload the page, however, now the default plugin is missing
on alternate reloads. :(

Comment 5

16 years ago
it looks like the problem with the patch might be that the failure code from
ReloadPlugins (in 'res') is being propagated all the way out of the method...

it seems like even if there is nothing to do nsPluginArrayImpl::Refresh() should
still return NS_OK...

Updated

16 years ago
Keywords: edt0.9.4
(Assignee)

Comment 6

16 years ago
Peter, I think the idea you were trying to implement in the patch when you 
skip page load if the list has not been changed will not work, because we kill 
all the plugins before reloading them including whatever plugin is currently 
running on the page, so we still need to reload the page to envoke the plugin 
again.
(Assignee)

Comment 7

16 years ago
Created attachment 64781 [details]
test case from the original problem -- illustration of the recursive load

Just for convenience.
(Assignee)

Comment 8

16 years ago
Created attachment 64783 [details] [diff] [review]
branch patch v.1

*** THIS PATCH IS FOR BRANCH ONLY ***

What it does.

If we will issue consecutive javascript refresh(1) commands then:
1. if plugins have not been chaged it will still reload  page every second time
and it will NOT rescan plugins at all
2. if you do change plugins before every refresh(1) command it will rescan
plugins and reload the page every time. This is the ACTUAL FIX.

How it works.

1. I calculate the check sum every time we scan for plugins, so it is done at
least once on start up. The check sum is now a memeber of |nsPluginHostImpl|.
2. A flag |mCalculatingCheckSumOnly| is introduced. If it is set then our
plugin scanning code will do what it normally does except actually adding
plugins to the plugin list. This way we can calculate the control sum of the
new plugin list BEFORE we create it.
3. The new to-be control sum is compared to the current value and thus we know
whether or not plugins have changed in the folder. If they have, we rescan them
and reload the page, otherwise -- we do not rescan plugins and reload the page
every second time.

Please advise if this behaviour would be acceptable.
(Assignee)

Comment 9

16 years ago
Downside: if plugins have changed, the refresh time doubles. But given this is 
supposed to be done relatively rarely, I think it is acceptable.
(Assignee)

Comment 10

16 years ago
Does anybody thinks this approach makes sense?

Comment 11

16 years ago
The checksum seems like a reasonable approach to me. Probably better than the
4.x method.

>1. if plugins have not been chaged it will still reload  page every second time
>and it will NOT rescan plugins at all.

Why every second time? Seems to me that with the checksum in place, we should be
able to reload everytime, as the user would expect.


(Assignee)

Comment 12

16 years ago
The original problem was recursive reloads. If we have page like this

  <html><body>
  <script language=javascript>
    navigator.plugins.refresh(1);
  </script>
  </body></html>

we cannot allow it to load every time. Just doing it every other time seems to 
help, but then we do plugins refresh every other time too, which is to my 
understanding what this bug is about. Recursive reloads is unrelated to plugins 
issue, it will happen on say document.reload() (do I remember the command 
correctly?). With document.reload it just counts and stops after 50 if I am not 
mistaken.

Comment 13

16 years ago
Ok, this sounds familiar. So, can we only reload if/when the checksum changes
then? That would probably even be better than every other time
(Assignee)

Comment 14

16 years ago
I thought about that too. Just wanted to keep changes minimal. Will look at it 
and if this is not much of a change, then we should probably do that.
(Assignee)

Comment 15

16 years ago
Created attachment 64944 [details] [diff] [review]
branch patch v.2

With this patch we don't do anything at all on refresh(1) -- neither refreshing
the plugin list, nor reloading the page, if plugins have not been changed.

In fact, the code in nsPluginArray.cpp is much cleaner now, I totally removed
the check for the last URL seen as an indication of possible recursion, looks
like we do not need it any more.

Please, review.
(Assignee)

Updated

16 years ago
Attachment #64783 - Attachment is obsolete: true

Comment 16

16 years ago
I don't have strong opinions either way, but checksums of dates are an inexact
measure of change. I can think of various scenerios where these numbers come out
the same but the plugins really need refresh...

- Unlikely possibility that two sets of dates have the same checksum.
- Installer that forces particular timestamp on files.
- Plugin that uses info in some *other* file as part of what gets registered.
- Use of this mechanim to force general autoreg (for xpcom components and for
xpt files - you are changing the rules here).

Also, '+=' can not be used on PRInt64 on all platforms. You should use the macros.
(Assignee)

Comment 17

16 years ago
> - Unlikely possibility that two sets of dates have the same checksum.

That's possible, agree, but unlikely to the almost 'impossible' extend.

> - Installer that forces particular timestamp on files.
> - Plugin that uses info in some *other* file as part of what gets registered.

If we don't have better solution, we can just evangelize these two.

> - Use of this mechanim to force general autoreg (for xpcom components and for
> xpt files - you are changing the rules here).

It was done some time ago by a specific request and there was a broad 
discussion on the issue. Arun, do you remember?

> Also, '+=' can not be used on PRInt64 on all platforms. You should use the > 
macros.

Ah... right, good point.
(Assignee)

Comment 18

16 years ago
Created attachment 64954 [details] [diff] [review]
branch patch v.3

Now using macros for 64 bit operations.
Attachment #64944 - Attachment is obsolete: true

Comment 19

16 years ago
Comment on attachment 64954 [details] [diff] [review]
branch patch v.3

r=peterl
Attachment #64954 - Flags: review+

Comment 20

16 years ago
Comment on attachment 64954 [details] [diff] [review]
branch patch v.3

sr=jband

NOTES:
lastModifiedTime should be initilized with LL_ZERO.

I'm not thrilled with overloading NS_ERROR_UNEXPECTED to mean a specific thing.
But, that can stand for now.

I trust you've done the appropriate amount of testing.
Attachment #64954 - Flags: superreview+
(Assignee)

Updated

16 years ago
Whiteboard: [PLUSME]

Comment 21

16 years ago
are we going to hit the trunk w/ this as well? please land on the 0.9.4 branch
by midnight tonight. once it lands there, please add "fixed0.9.4" to the keyword
field.

Comment 22

16 years ago
This should land on the trunk as well. Though, like jband, I don't like the
NS_ERROR_UNEXPECTED thing. I think that the trunk solution should include adding
a PRBool * parameter to the ReloadPlugins() method for the "pluginsNotChanged"
variable to be returned in.

Updated

16 years ago
Keywords: edt0.9.4 → edt0.9.4+
(Assignee)

Comment 23

16 years ago
I plan to add a new error code for the trunk patch, our error codes are module 
based and I think this would be right thing to do. Something like 
NS_ERROR_PLUGIN_PLUGUNS_NOT_RESCANNED.

And no, this patch is not for the trunk. On the trunk it will be different 
because we have dp's caching mechanism there implemented so we will use that 
instead of physical directory scan for timestamps.
(Assignee)

Comment 24

16 years ago
Patch in the branch. We should keep this bug open untill we fix the problem on 
the trunk too.

Shrirang, to test the fix use the testcase jband attached. You should see 
nothing just pressing the button. But if every time before pressing the button 
you change something in the plugins folder, you will see the page reloaded (and 
plugin refreshed, although I don't know how to verify this).
Keywords: fixed0.9.4
Whiteboard: [PLUSME]
(Assignee)

Comment 25

16 years ago
Created attachment 65113 [details]
This test case will allow you to see everything

Comment 26

16 years ago
Thx for the tip,Andrei ! I will check this fix in tomorrow's build.

Comment 27

16 years ago
Using the latest respin - 0115 branch (0.9.4) , this works just fine now. I see 
a plugin refresh 'every time' I change/edit the plugins and do a 'refresh'. 
Tried a variety of plugin addition/deletion and checked the result. Verified on 
the branch.
Keywords: verified0.9.4
(Assignee)

Comment 28

16 years ago
We should get it to 0.9.8.
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 29

16 years ago
Created attachment 65795 [details] [diff] [review]
trunk patch v.2

This patch detects changes in plugins before trying to reload plugins info on
navigator.plugins.refresh(1) command. If no changes found the plugin info list
is not refreshed and the page is not reloaded.

Since the trunk code base uses dp's plugin info caching mechanism, and this
caching is sort of trying to detect changes too, I used it to set flags
indicating that plugins have been changed. The patch detects all tree possible
types of changes: plugin addition, plugin removal and plugin update.

Please, review.
Attachment #64674 - Attachment is obsolete: true
(Assignee)

Comment 30

16 years ago
Arun, with this patch no xpcom components will be refreshed with 
navigator.pluigns.refresh(1) command if Plugins folder content did not change. 
However, they will still refresh on navigator.plugins.refresh(0) command. I 
know, there are different opinions on refreshing components at all with this 
javascript command.

Comment 31

16 years ago
Comment on attachment 65795 [details] [diff] [review]
trunk patch v.2

r=peterl
Attachment #65795 - Flags: review+

Comment 32

16 years ago
I am a little confussed on the status of this one: Removing KWs: edt0.9.4+,
fixed0.9.4, verified0.9.4

It was verified fixed on 1-16, yet there seems to be more code changes under review.
Keywords: edt0.9.4+, fixed0.9.4, verified0.9.4 → edt0.9.4
(Assignee)

Comment 33

16 years ago
Patches for the branch and the trunk are completely different. We are trying to 
get it on the trunk now.

Comment 34

16 years ago
Michael,
Pls see  "------- Additional Comment #8 From av 2002-01-13 23:56 -------"

Seems like the patch was only for the branch. And the trunk was going to get a 
new patch...

Comment 35

16 years ago
Trunk patch: all plugin caching code changes look ok to me. r=dp
Keywords: edt0.9.4 → edt0.9.4+, fixed0.9.4, verified0.9.4

Comment 36

16 years ago
Comment on attachment 65795 [details] [diff] [review]
trunk patch v.2

In trunk patch v.2, this line:

      if(checkForUnwantedPlugins && isUnwantedPlugin(pluginTag))

appears twice in the patch. I think the second one is unnecessary. At the very
least, you can cache the test in a PRBool. Other than that,

sr=beard
Attachment #65795 - Flags: superreview+

Comment 37

16 years ago
-> 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Reporter)

Comment 38

16 years ago
I am seeing behaviour that now causes the browser to reproduce bug 93351,  
global javascript refresh calls endlessly looping.  This can be reproduced by 
enabling the JRE scan and having the plug-ins installed locally.
(Reporter)

Comment 39

16 years ago
Err, just to clarify having JRE scanned from the system and having the plug-ins 
installed in your local plugins directory.  Even though the plug-ins aren't 
being modified, whether being added or deleted, the new checksum never matches 
up to the known one.
(Assignee)

Comment 40

16 years ago
Todd, are you talking about the 0.9.4 branch?
(Reporter)

Comment 41

16 years ago
Yes, actually there is now bugscape bug for this issue, 11998.
(Assignee)

Updated

16 years ago
Blocks: 78914
(Assignee)

Comment 42

16 years ago
Patch (modified to resolve conflicts) checked in. Marking fixed.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 43

16 years ago
v
Status: RESOLVED → VERIFIED

Updated

14 years ago
Keywords: fixed0.9.4
You need to log in before you can comment on or make changes to this bug.