285738 - Reload incorrectly disabled on certain pages

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050311
Firefox/1.0+

I'm pretty certain this is a regression caused by Bug 285404.  When visiting
certain cites, the reload function is disabled when it shouldn't be.  Below are
a few more links.  What's interesting to note about the Slashdot links is that
while some articles have this issue, others don't. The articles which do have
the problem have it reproduceably, however.

http://slashdot.org/article.pl?sid=05/03/11/0143211&tid=129&tid=1
http://yro.slashdot.org/article.pl?sid=05/03/11/011203&tid=141&tid=123

Comment 1

[Bill Gianopoulos [:WG9s]]

I can confirm the behavior and also exhonerate bug 285404 as the culprit.  This
regressed between the 2005030807 and 2005030907 builds.  The patch for bug
285404 was not in the 2005030907 build which exhibits this problem.

Comment 2

[Bill Gianopoulos [:WG9s]]

Backing out the checkin for bug 284993 appears to correct this issue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

jst, darin, any idea what's up here?  Sounds like the OnLocationChange
notifications are being missed or something...

Comment 4

[Bill Gianopoulos [:WG9s]]

I suspect there may be some relation to ads in iframes based on pages on which I
see this issue.  I tried a clean profile without any ad blocking and the problem
persists.

Comment 5

[Martijn Wargers (dead)]

Attachment: Testcase

The bug happens for me with this testcase.

Comment 6

[Jerry Baker]

Verifying test case "works" on WinXP.

Comment 7

[Bill Gianopoulos [:WG9s]]

(In reply to comment #6)
> Verifying test case "works" on WinXP.

I think he is trying to say the testcase successfuly duplicates the problem.  It
certianly does for me under WinXP.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Also note that this is not an issue for SeaMonkey....  Where's the Firefox code
that disables the reload button in the first place live?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3183

Comment 10

[Radek 'sysKin' Czyz]

(In reply to comment #5)
> Created an attachment (id=177923) [edit]
> Testcase
> 
> The bug happens for me with this testcase.

I just tried this testcase and Firefox crashed. TB4614855X. Might be a
coincidence of course.

Comment 11

[timeless]

[e-mail]: that report has no sybmols, please try to find a firefox
which when crashed gives symbols. (good luck)

Comment 12

[Bill Gianopoulos [:WG9s]]

I know what is going on here.  Patch forthcoming.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase showing that this is a bug in the Firefox UI

Isn't that Firefox code just buggy?  Consider behavior on this testcase...

Comment 14

[Bill Gianopoulos [:WG9s]]

Attachment: proposed patch

This issue is caused by the fix for bug 284993 changing the code from not
firing On Location notification in the sub-frame case to firing them
unconditionally.  Unfortuately, I could not figure out a clean way to restore
the old behavior and resorted to setting a flag called
mShouldFireOnLocationChange in SetCurrentURI, if it would have fired an on
location change were it not for the flag setting added by 284993 being false. 
Then in CreateContentViewer I changed the uncondtional call to
FireOnLocationChange to be conditional based on the setting of
mShouldFireOnLocationChange.

I am welcome to suggestion on a cleaner way to do this or a better patch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

What about what Boris said in bug 237776? Is it possible that moving the
original code from that patch to a different spot will take care of this bug?

Comment 16

[Bill Gianopoulos [:WG9s]]

(In reply to comment #15)
> What about what Boris said in bug 237776? Is it possible that moving the
> original code from that patch to a different spot will take care of this bug?

Could be, but the code here is wrong in any event.  The intent of the patch for
bug 284993 was to delay firing the on location change until the page was more
fully loaded.  It ended up altering the logic in a way that on location changes
were fired when they formerly were not.

Comment 17

[Bill Gianopoulos [:WG9s]]

Should have mentioned this patch also fixes some other issues I have noticed in
recent nightlies concerning sites using frames.  Particularly in yahoo mail.  If
you click on the link to Inbox on the my yahoo mpage, it would load the Inbox
view and then immediatley switch to the page the Check Mail link is supposed to
go to.  I had kind of thought that was a server issue, but I guess not.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ok.  So let me see whether I understand this... We used to _not_ fire
onLocationChange in two cases:

1)  Error page load (because of the early return up front in SetCurrentURI).
2)  Initial creation of iframes/frames (due to the hack in SetCurrentURI() later
    on).

Seems to me, since the return value of OnLoadingSite is universally ignored, the
right thing to do to restore this behavior would be to return a boolean from
OnNewURI and OnLoadingSite indicating whether the caller should fire
OnLocationChange (and make sure to return false any time PR_TRUE is passed in or
some such).

Comment 19

[Johnny Stenback (:jst)]

Yeah, that sounds like the right thing to do. William, you wanna come up with a
new patch, or you want me to?

Comment 20

[Bill Gianopoulos [:WG9s]]

Attachment: patch following bz's suggestion

New patch following BZ's suggestion.

Comment 21

[Bill Gianopoulos [:WG9s]]

(In reply to comment #13)
> Created an attachment (id=178827) [edit]
> Testcase showing that this is a bug in the Firefox UI
> 
> Isn't that Firefox code just buggy?  Consider behavior on this testcase...

I'd like to just fix the regression caused by bug 284993 here.  I think for this
problematic code which was checked in to fix bug 237776, we should either reopen
that bug or file a new bug.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178875 [details] [diff] [review]
patch following bz's suggestion

>Index: mozilla/docshell/base/nsDocShell.h
>+    PRBool OnLoadingSite(nsIChannel * aChannel,
>                            PRBool aFireOnLocationChange);
>+    PRBool OnNewURI(nsIURI * aURI, nsIChannel * aChannel, PRUint32 aLoadType,
>                   PRBool aFireOnLocationChange);
>-    void SetCurrentURI(nsIURI *aURI, nsIRequest *aRequest,
>+    PRBool SetCurrentURI(nsIURI *aURI, nsIRequest *aRequest,
>                        PRBool aFireOnLocationChange);

Fix indents?  And document what the return value means?  And how it interacts
with the last arg?

>Index: mozilla/docshell/base/nsDocShell.cpp
> nsDocShell::SetCurrentURI(nsIURI *aURI, nsIRequest *aRequest,
>                           PRBool aFireOnLocationChange)
>+// Returns PR_TRUE if the caller is responsible for ensuring
>+// FireOnLocationChange is called; PR_FALSE otherwise.

This is the wrong place for this comment.

>     if (aFireOnLocationChange) {
>         FireOnLocationChange(this, aRequest, aURI);
>+        return PR_FALSE;
>     }
>+    return PR_TRUE;

How about just return !aFireOnLocationChange at the end?

> nsDocShell::CreateContentViewer(const char *aContentType,
>+    PRBool OnLocationChangeNeeded = PR_FALSE;

Lowercase that first "On"?  And declare it where it's first used?

> nsDocShell::OnNewURI(nsIURI * aURI, nsIChannel * aChannel,

>+    PRBool OnLocationChangeNeeded = PR_FALSE;

Same.

>+PRBool
> nsDocShell::OnLoadingSite(nsIChannel * aChannel, PRBool aFireOnLocationChange)
>     NS_ENSURE_TRUE(uri, NS_ERROR_FAILURE);

Except this function returns a PRBool....

Comment 23

[Bill Gianopoulos [:WG9s]]

Attachment: take 3

I think this addresses BZ's comments on the previous patch.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178890 [details] [diff] [review]
take 3

>Index: mozilla/docshell/base/nsDocShell.h
>+    // Returns PR_TRUE if would have called FireOnCocationChange,

"Cocation"

>+    // but did not because aFireOnLocationChange was false on entry.
>+    // In this case it is the callers responsibility to ensure

"caller's"

>+    // Returns PR_TRUE if would have called FireOnCocationChange,

Again.

>+    // In this case it is the callers responsibility to ensure

And here.

>+    // Returns PR_TRUE if would have called FireOnCocationChange,
>+    // In this case it is the callers responsibility to ensure

And here.

sr=bzbarsky with those fixed, assuming that none of those long lines go over
the 80-char limit...

Comment 25

[Bill Gianopoulos [:WG9s]]

Attachment: Addressing sr review comments

Carrying sr+ over from pevious patch.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 178924 [details] [diff] [review]
Addressing sr review comments

r=jst. Thanks for fixing!

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in that last patch.