Closed Bug 285738 Opened 19 years ago Closed 19 years ago

Reload incorrectly disabled on certain pages

Categories

(SeaMonkey :: Tabbed Browser, defect)

x86
Windows XP
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: RyanVM, Assigned: wgianopoulos)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

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
Flags: blocking-aviary1.1?
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.
Backing out the checkin for bug 284993 appears to correct this issue.
Depends on: 284993
No longer depends on: 285404
jst, darin, any idea what's up here?  Sounds like the OnLocationChange
notifications are being missed or something...
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.
Attached file Testcase
The bug happens for me with this testcase.
Verifying test case "works" on WinXP.
(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.

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?
(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.
syskin@syskin.cjb.net: that report has no sybmols, please try to find a firefox
which when crashed gives symbols. (good luck)
I know what is going on here.  Patch forthcoming.
Assignee: bugs → wgianopoulos
Component: Toolbars → Tabbed Browser
Product: Firefox → Core
Isn't that Firefox code just buggy?  Consider behavior on this testcase...
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #178829 - Flags: superreview?(darin)
Attachment #178829 - Flags: review?(jst)
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?
(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.

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.
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).
Yeah, that sounds like the right thing to do. William, you wanna come up with a
new patch, or you want me to?
Attached patch patch following bz's suggestion (obsolete) — Splinter Review
New patch following BZ's suggestion.
Attachment #178829 - Attachment is obsolete: true
Attachment #178875 - Flags: superreview?(bzbarsky)
Attachment #178875 - Flags: review?(jst)
(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 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....
Attachment #178875 - Flags: superreview?(bzbarsky) → superreview-
Attachment #178875 - Attachment description: patch followin bz's suggestion → patch following bz's suggestion
Attached patch take 3 (obsolete) — Splinter Review
I think this addresses BZ's comments on the previous patch.
Attachment #178875 - Attachment is obsolete: true
Attachment #178890 - Flags: superreview?(bzbarsky)
Attachment #178890 - Flags: review?(jst)
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...
Attachment #178890 - Flags: superreview?(bzbarsky) → superreview+
Carrying sr+ over from pevious patch.
Attachment #178890 - Attachment is obsolete: true
Attachment #178924 - Flags: superreview+
Attachment #178924 - Flags: review?(jst)
Attachment #178924 - Flags: superreview+
Comment on attachment 178924 [details] [diff] [review]
Addressing sr review comments

r=jst. Thanks for fixing!
Attachment #178924 - Flags: review?(jst) → review+
Checked in that last patch.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #178829 - Flags: superreview?(darin)
Attachment #178829 - Flags: review?(jst)
Attachment #178875 - Flags: review?(jst)
Attachment #178890 - Flags: review?(jst)
Flags: blocking-aviary1.1?
QA Contact: bugzilla → tabbed-browser
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: