Reload loop on navigator.plugins.refresh(true)

VERIFIED FIXED in mozilla0.9.4



18 years ago
17 years ago


(Reporter: serhunt, Assigned: serhunt)



Windows NT

Firefox Tracking Flags

(Not tracked)




(4 attachments)



18 years ago
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.

Comment 1

18 years ago
Created attachment 44488 [details]
test case


18 years ago
Priority: -- → P2
Target Milestone: --- → mozilla0.9.4


18 years ago
Blocks: 90946

Comment 2

18 years ago
Created attachment 44489 [details] [diff] [review]
possible solution -- first try

Comment 3

18 years ago
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)

Comment 4

18 years ago
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.

Comment 5

18 years ago
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.

Comment 6

18 years ago
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;
    if (presShell) {
      nsCOMPtr<nsIDocument> doc;
      if (doc) {
        nsCOMPtr<nsIURI> url;
        if (url) {
          char * urlCStr;
          if (urlCStr) {
            nsAutoString urlStr;
            *aURLStr = urlStr.ToNewUnicode();

Comment 7

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

Comment 8

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

Comment 9

18 years ago
Created attachment 44574 [details] [diff] [review]
second try -- better solution, no ugly timing assumptions

Comment 10

18 years ago

Comment 11

18 years ago
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.

Comment 12

18 years ago
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?

Comment 13

18 years ago
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 
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.

Comment 15

18 years ago
Created attachment 45008 [details] [diff] [review]
third try -- jst suggestions addressed, much simpler and cleaner now

Comment 16

18 years ago
r=peterl on new patch.

Comment 17

18 years ago
Nominating for topembed as it blocks another topembed bug.
Keywords: topembed
sr=jst, much better :-)

Comment 19

18 years ago
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.

Comment 21

18 years ago
Checked in to the trunk.
Whiteboard: [FIXED IN TRUNK]

Comment 22

18 years ago
in addition to topembed, adding nsenterprise and nsbranch.  this one ought to be
all around :-)
Keywords: nsBranch, nsenterprise

Comment 23

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

Comment 24

18 years ago
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 

Comment 25

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

Comment 26

18 years ago
Checked in.
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 27

18 years ago
works great on the branch 0822 too. 
Keywords: nsBranch → nsbranch

Comment 28

17 years ago
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:

and reloading should work fine.
Resolution: FIXED → ---

Comment 29

17 years ago
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.

Comment 30

17 years ago
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.

Comment 31

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

Comment 32

17 years ago
Bug 119621 already filed. Closing this one.
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED

Comment 33

17 years ago
You need to log in before you can comment on or make changes to this bug.