Closed
Bug 285738
Opened 19 years ago
Closed 19 years ago
Reload incorrectly disabled on certain pages
Categories
(SeaMonkey :: Tabbed Browser, defect)
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
Reporter | ||
Updated•19 years ago
|
Flags: blocking-aviary1.1?
Assignee | ||
Comment 1•19 years ago
|
||
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.
Assignee | ||
Comment 2•19 years ago
|
||
Backing out the checkin for bug 284993 appears to correct this issue.
Comment 3•19 years ago
|
||
jst, darin, any idea what's up here? Sounds like the OnLocationChange notifications are being missed or something...
Assignee | ||
Comment 4•19 years ago
|
||
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•19 years ago
|
||
The bug happens for me with this testcase.
Comment 6•19 years ago
|
||
Verifying test case "works" on WinXP.
Assignee | ||
Comment 7•19 years ago
|
||
(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•19 years ago
|
||
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?
Reporter | ||
Comment 9•19 years ago
|
||
http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3183
Comment 10•19 years ago
|
||
(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•19 years ago
|
||
syskin@syskin.cjb.net: that report has no sybmols, please try to find a firefox which when crashed gives symbols. (good luck)
Assignee | ||
Comment 12•19 years ago
|
||
I know what is going on here. Patch forthcoming.
Assignee: bugs → wgianopoulos
Component: Toolbars → Tabbed Browser
Product: Firefox → Core
Comment 13•19 years ago
|
||
Isn't that Firefox code just buggy? Consider behavior on this testcase...
Assignee | ||
Comment 14•19 years ago
|
||
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)
Reporter | ||
Comment 15•19 years ago
|
||
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?
Assignee | ||
Comment 16•19 years ago
|
||
(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.
Assignee | ||
Comment 17•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Yeah, that sounds like the right thing to do. William, you wanna come up with a new patch, or you want me to?
Assignee | ||
Comment 20•19 years ago
|
||
New patch following BZ's suggestion.
Attachment #178829 -
Attachment is obsolete: true
Attachment #178875 -
Flags: superreview?(bzbarsky)
Attachment #178875 -
Flags: review?(jst)
Assignee | ||
Comment 21•19 years ago
|
||
(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•19 years ago
|
||
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-
Assignee | ||
Updated•19 years ago
|
Attachment #178875 -
Attachment description: patch followin bz's suggestion → patch following bz's suggestion
Assignee | ||
Comment 23•19 years ago
|
||
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 24•19 years ago
|
||
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+
Assignee | ||
Comment 25•19 years ago
|
||
Carrying sr+ over from pevious patch.
Attachment #178890 -
Attachment is obsolete: true
Attachment #178924 -
Flags: superreview+
Attachment #178924 -
Flags: review?(jst)
Assignee | ||
Updated•19 years ago
|
Attachment #178924 -
Flags: superreview+
Comment 26•19 years ago
|
||
Comment on attachment 178924 [details] [diff] [review] Addressing sr review comments r=jst. Thanks for fixing!
Attachment #178924 -
Flags: review?(jst) → review+
Comment 27•19 years ago
|
||
Checked in that last patch.
Assignee | ||
Updated•19 years ago
|
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #178829 -
Flags: superreview?(darin)
Attachment #178829 -
Flags: review?(jst)
Updated•19 years ago
|
Attachment #178875 -
Flags: review?(jst)
Updated•19 years ago
|
Attachment #178890 -
Flags: review?(jst)
Updated•19 years ago
|
Flags: blocking-aviary1.1?
Updated•19 years ago
|
QA Contact: bugzilla → tabbed-browser
Updated•16 years ago
|
Product: Core → SeaMonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•