93351 - Reload loop on navigator.plugins.refresh(true)

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[av (gone)]

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

[av (gone)]

Attachment: test case

Comment 2

[av (gone)]

Attachment: possible solution -- first try

Comment 3

[av (gone)]

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

[Peter Lubczynski]

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

[av (gone)]

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

[Peter Lubczynski]

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);
          }
        }
      }
    }
  }

Comment 7

[av (gone)]

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

Comment 8

[Peter Lubczynski]

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

Comment 9

[av (gone)]

Attachment: second try -- better solution, no ugly timing assumptions

Comment 10

[Peter Lubczynski]

r=peterl

Comment 11

[Brian Nesse (gone)]

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

[av (gone)]

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

[av (gone)]

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.

Comment 14

[Johnny Stenback (:jst)]

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

[av (gone)]

Attachment: third try -- jst suggestions addressed, much simpler and cleaner now

Comment 16

[Peter Lubczynski]

r=peterl on new patch.

Comment 17

[av (gone)]

Nominating for topembed as it blocks another topembed bug.

Comment 18

[Johnny Stenback (:jst)]

sr=jst, much better :-)

Comment 19

[av (gone)]

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.

Comment 20

[Johnny Stenback (:jst)]

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

Comment 21

[av (gone)]

Checked in to the trunk.

Comment 22

[Arun Ranganathan]

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

Comment 23

[av (gone)]

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

Comment 24

[shrirang khanzode]

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)

Comment 25

[Marek Z. Jeziorek]

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

Comment 26

[av (gone)]

Checked in.

Comment 27

[shrirang khanzode]

works great on the branch 0822 too. 

Comment 28

[Doug Turner (:dougt)]

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.

Comment 29

[av (gone)]

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

[TaylorToddK]

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

[Marten ter Borgh]

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

Comment 32

[av (gone)]

Bug 119621 already filed. Closing this one.

Comment 33

[shrirang khanzode]

v