549 bytes, text/html
117 bytes, text/html
7.90 KB, patch
Peter Lubczynski: review+
John Bandhauer: superreview+
|Details | Diff | Splinter Review|
674 bytes, text/html
30.19 KB, patch
Peter Lubczynski: review+
Patrick C. Beard: superreview+
|Details | Diff | Splinter Review|
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.
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?
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. :(
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.
Created attachment 64781 [details] test case from the original problem -- illustration of the recursive load Just for convenience.
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.
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.
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.
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.
Created attachment 64954 [details] [diff] [review] branch patch v.3 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.
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.
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
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.