1. create an html file with the above mentioned JavaScript command (I'll attach) 2. load it to Mozilla 3. watch it keeping reloading Expected: although behaviour is logical, 4.x and IE don't do this. We should prevent recursive reloading.

The patch just checks for time passed since previous similar commad -- reload originating from JavaScript. If this time is small, it doesn't perform refresh and reload. If we decide to go with this sort of solution we should agree on: 1. What time is small enough? I deliberately set it to 5 seconds 2. Maybe check for URL and bypass reload if(time_is_small AND URL_is_the_same)

Wow, what a hack. I'm thinking that just doing the "URL_is_the_same" test may be enough as long as the "last URL" is cleared in the |else| fragment and perhaps in a few other places. That would stop the recursion but would still work when reloaded manually.

You are right, the more I think about it the more it seems natural to me. It will be simpler and less hacky. I'll do it tomorrow or maybe on Monday since I leave the town Friday afternoon. Yes, it will work correctly when user quickly navigates to the same page and we will not rely on obscure timing assumptions. Good idea.

I didn't see a simple way of getting the URL from the docshell but thanks to Don Cone, here is a sample of how to do it: if (docShell) { nsCOMPtr<nsIPresShell> presShell; docShell->GetPresShell(getter_AddRefs(presShell)); if (presShell) { nsCOMPtr<nsIDocument> doc; presShell->GetDocument(getter_AddRefs(doc)); if (doc) { nsCOMPtr<nsIURI> url; doc->GetDocumentURL(getter_AddRefs(url)); if (url) { char * urlCStr; url->GetSpec(&urlCStr); if (urlCStr) { nsAutoString urlStr; urlStr.AssignWithConversion(urlCStr); *aURLStr = urlStr.ToNewUnicode(); nsMemory::Free(urlCStr); } } } } }

Hm... Doesn't nsIWebNavigation object give us url? I was planning using it. Maybe I was wrong, thanks, Peter.

Doh! nsIWebNavigation does expose the current url. That's much easier.

r=peterl

To answer the question peter posed in bug 90946. 4.x seems to do this in a bi-level approach. npn_reloadplugins takes a parameter |reloadPages|, calls the FrontEnd to refresh the plugin list. The real trick however appears to be here... Before calling the FE to reload the plugins, it saves the head of the plugin list. Based on the premise that new plugins are added to the head of the list, it compares the current value to the old value after returning from the refresh. If the list hasn't changed, it doesn't reload the page.

So, it doesn't nuke the list before refresh. What happens if plugin is removed? I don't think we can use this approach, because we destroy the list completely and create a new one. But I like the idea to see if the list remains essentially the same. May be introduce some sort of check sum?

No, will still reload the libraries even if there is no change in plugins so we must to reload the page if we decided to refresh the plugins. The latest patch stays.

A few suggestions, why not simply hold on to the last nsIURI in stead of holding on to the string representation of the URI? If you'd do that you could use a nsCOMPtr<nsIURI> mLastURI and you wouldn't need to worry about the cleanup of the string, and you could do the equality check by simply using nsIURI::Equals() with the last and current uri's. Other comments about the patch: + if(mDocShell) + webNav = do_QueryInterface(mDocShell); No need to check for null mDocShell here, do_QueryInterface() is null safe. This does one extra allocation and string copy: + nsXPIDLCString uriString; + uri->GetSpec(getter_Copies(uriString)); + if(mLastURL) + nsCRT::free(mLastURL); + + mLastURL = PL_strdup(uriString.get()); Change that to: + if(mLastURL) + nsCRT::free(mLastURL); + + uri->GetSpec(&mLastURL); would save you the extra allocation and string copy. 'else after return' here: + if(mLastURL) { + if(0 == PL_strcmp(uriString.get(), mLastURL)) { + nsCRT::free(mLastURL); + mLastURL = nsnull; + return res; + } + else { + nsCRT::free(mLastURL); + mLastURL = nsnull; + } No need to put that 'else' there. Please attach a new patch, either hold on to the nsIURI, or fix the problems pointed out above.

r=peterl on new patch.

Nominating for topembed as it blocks another topembed bug.

sr=jst, much better :-)

This patch is going to introduce a minor problem. It will work fine if the body of the page contains <script>navigator.plugins.refresh(true)</script>, but if it is <a href="javascript:navigator.plugins.refresh(true)">refresh</a> instead, and user clicks on this link several times in a row, it will do refresh/reload only every second time. I don't see any way to distinguish these two cases in this code in order to take this into account, but beleive that the severity of this problem is much less than that of the original one, and we should proceed with this patch, filing another bug. Peter, Johnny, if you agree, I will check it in. Or maybe you give me an idea how to do this right.

Works for me, but please file a new bug on the weird behavior described above.

Checked in to the trunk.

in addition to topembed, adding nsenterprise and nsbranch. this one ought to be all around :-)

So, where else it should go? 0.9.2? Do I need a permission? topembed+? Where else?

verified the fix on windows NT with build 0810. Testcase page no longer reloads if the refresh code is embeddd in the body. However, I see the problem with the refresh code being in a link that Andrei mentioned (new bug already filed for that)

Andrei, please check it into 0.9.2 (and resolve it at that time). Thanks

Checked in.

works great on the branch 0822 too.

This fix cause regressions. "I am noticing a really strange behaviour with regards to plug-ins... if I do a javascript:refreshplugins(), it only actually succeeds every other time. For some reason, even though the calls reports success, doing a NPN_GetURL(m_pNPInstance, "javascript:navigator.plugins.refresh(true)", "_self"); only works twice." I trace it back to this patch. I think that a better fix to break loops is to keep a counter of the number of reload of the same URL. If it is greater than some n, we fail. In this way, "javascript:navigator.plugins.refresh(true)" url load will work correctly. Moreover, do we even care if a page decides to refresh forever via the navigator.plugins api? IMO, we do not need this kind of "protection" at the plugin level, but, if needed at all, it should be in the document loader. Todd, this is the problem we saw. remove: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/dom/src/base&command=DIFF_FRAMESET&file=nsPluginArray.cpp&rev2=1.18&rev1=1.17 and reloading should work fine.

This was a known issue at the time the patch was introduced (see comment #19) but it has been decided that this problem is minor enough to let the patch in.

I think this is more than a minor issue. This could dramatically affect the npnul32 plug-in retrieval mechanism. This opens the possibility of a plug-in being installed but not appearing in the content when they user tries to refresh the plug-ins. In addition, the npnul32 won't be informed that its refresh call failed.

av, could you please file a bug per comment #19 and close this bug?

Bug 119621 already filed. Closing this one.

v