119621 - Plug-ins refreshed on page on every other refresh interval

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[TaylorToddK]

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

[John Bandhauer]

Attachment: 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

[John Bandhauer]

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

[Peter Lubczynski]

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

[Peter Lubczynski]

Attachment: 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

[rpotts (gone)]

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...

Comment 6

[av (gone)]

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.

Comment 7

[av (gone)]

Attachment: test case from the original problem -- illustration of the recursive load

Just for convenience.

Comment 8

[av (gone)]

Attachment: 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.

Comment 9

[av (gone)]

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

Comment 10

[av (gone)]

Does anybody thinks this approach makes sense?

Comment 11

[Brian Nesse (gone)]

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.

Comment 12

[av (gone)]

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

[Brian Nesse (gone)]

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

Comment 14

[av (gone)]

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.

Comment 15

[av (gone)]

Attachment: 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.

Comment 16

[John Bandhauer]

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.

Comment 17

[av (gone)]

> - 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.

Comment 18

[av (gone)]

Attachment: branch patch v.3

Now using macros for 64 bit operations.

Comment 19

[Peter Lubczynski]

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

r=peterl

Comment 20

[John Bandhauer]

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.

Comment 21

[Judson Valeski]

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

[Brian Nesse (gone)]

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.

Comment 23

[av (gone)]

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.

Comment 24

[av (gone)]

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).

Comment 25

[av (gone)]

Attachment: This test case will allow you to see everything

Comment 26

[shrirang khanzode]

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

Comment 27

[shrirang khanzode]

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.

Comment 28

[av (gone)]

We should get it to 0.9.8.

Comment 29

[av (gone)]

Attachment: 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.

Comment 30

[av (gone)]

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

[Peter Lubczynski]

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

r=peterl

Comment 32

[Michael Dunn]

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.

Comment 33

[av (gone)]

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

Comment 34

[shrirang khanzode]

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

[Suresh Duddi (gone)]

Trunk patch: all plugin caching code changes look ok to me. r=dp

Comment 36

[Patrick C. Beard]

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

Comment 37

[Patrick C. Beard]

-> 0.9.9

Comment 38

[TaylorToddK]

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.

Comment 39

[TaylorToddK]

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.

Comment 40

[av (gone)]

Todd, are you talking about the 0.9.4 branch?

Comment 41

[TaylorToddK]

Yes, actually there is now bugscape bug for this issue, 11998.

Comment 42

[av (gone)]

Patch (modified to resolve conflicts) checked in. Marking fixed.

Comment 43

[shrirang khanzode]

v