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.

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.

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.

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?

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

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

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.

Just for convenience.

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

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

Does anybody thinks this approach makes sense?

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.

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.

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

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.

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.

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.

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

Now using macros for 64 bit operations.

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

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.

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.

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.

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.

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

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

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.

We should get it to 0.9.8.

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.

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 on attachment 65795 [details] [diff] [review] trunk patch v.2 r=peterl

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.

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

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

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

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

-> 0.9.9

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.

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.

Todd, are you talking about the 0.9.4 branch?

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

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

v