Reload loop on navigator.plugins.refresh(true)

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
Plug-ins
P2
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: av (gone), Assigned: av (gone))

Tracking

({topembed})

Trunk
mozilla0.9.4
x86
Windows NT
topembed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [VERIFIED ON TRUNK] [VERIFIED ON BRANCH], URL)

Attachments

(4 attachments)

(Assignee)

Description

17 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.
(Assignee)

Comment 1

17 years ago
Created attachment 44488 [details]
test case
(Assignee)

Updated

17 years ago
Priority: -- → P2
Target Milestone: --- → mozilla0.9.4
(Assignee)

Updated

17 years ago
Blocks: 90946
(Assignee)

Comment 2

17 years ago
Created attachment 44489 [details] [diff] [review]
possible solution -- first try
(Assignee)

Comment 3

17 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

17 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.
(Assignee)

Comment 5

17 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

17 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;
    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);
          }
        }
      }
    }
  }
(Assignee)

Comment 7

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

Comment 8

17 years ago
Doh! nsIWebNavigation does expose the current url. That's much easier.
(Assignee)

Comment 9

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

Comment 10

17 years ago
r=peterl

Comment 11

17 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.
(Assignee)

Comment 12

17 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?
(Assignee)

Comment 13

17 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 
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.
(Assignee)

Comment 15

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

Comment 16

17 years ago
r=peterl on new patch.
(Assignee)

Comment 17

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

Comment 19

17 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.
(Assignee)

Comment 21

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

Comment 22

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

Comment 23

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

Comment 24

17 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 
that)
Whiteboard: [FIXED IN TRUNK] → [VERIFIED ON TRUNK]

Comment 25

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

Comment 26

17 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [VERIFIED ON TRUNK] → [VERIFIED ON TRUNK] [FIXED ON BRANCH]

Comment 27

17 years ago
works great on the branch 0822 too. 
Status: RESOLVED → VERIFIED
Keywords: nsBranch → nsbranch
Whiteboard: [VERIFIED ON TRUNK] [FIXED ON BRANCH] → [VERIFIED ON TRUNK] [VERIFIED ON BRANCH]

Comment 28

16 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:

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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 29

16 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

16 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

16 years ago
av, could you please file a bug per comment #19 and close this bug?
(Assignee)

Comment 32

16 years ago
Bug 119621 already filed. Closing this one.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago16 years ago
Resolution: --- → FIXED

Comment 33

16 years ago
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.