Closed Bug 302115 Opened 19 years ago Closed 19 years ago

"Try again" on XUL error page loads wrong thing after going back

Categories

(Core :: DOM: Navigation, defect, P1)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8beta4

People

(Reporter: MatsPalmgren_bugz, Assigned: Biesinger)

References

(
URL
)

Details

(Whiteboard: [ETA 8/6])

Attachments

(1 file, 1 obsolete file)

merged to trunk
(deleted), patch
bzbarsky
: approval-branch-1.8.1-
Details | Diff | Splinter Review 
"Try again" on "Net Timeout Error" page loads wrong URL

STEPS TO REPRODUCE
1. make a request for http://1.2.3.4/ and wait for it to timeout...
   (the "Net Timeout Error" page is presented.)
2. make a request that will succeed, e.g. http://www.mozilla.org/
3. go Back and wait for it to timeout again
4. click on "Try again"

ACTUAL RESULTS
The URL entered in step 2 loads

EXPECTED RESULTS
The URL entered in step 1 should be requested again, ideally without
killing forward history.

PLATFORMS & BUILDS TESTED
SeaMonkey nightly build 2005-07-23-01 trunk Linux (100% reproducible)
is that also an issue with bfcache disabled?
The bug also occurs with
user_pref("browser.sessionhistory.max_entries", 0);
user_pref("browser.sessionhistory.max_viewers", 0);

I see the same on WinXP. 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050725
Firefox/1.0+ ID:2005072521
WFM with Mozilla 1.8b1 (XUL error pages enabled) but reproducible with Deer Park a2.
The trunk XUL error page is a re-write, or at least a rearrangement. The "Try
again" button doesn't explicitly load the error URL (although we know it), we do
a location.reload() instead (according to the code comments this is in case
there's postdata). If you go back from some page to an error page the "try
again" button loads the page you came back from.

The odd thing is that location.href shows the intended URL, but
location.reload() does something else anyway. But only for error pages; in the
regular case after a back reload does refresh the page you're on.

turning off bfcache (assuming browser.sessionhistory.max_viewers=0 is off) made
no difference.
Assignee: adamlock → benjamin
Flags: blocking1.8b4?
Flags: blocking-aviary1.5+
Summary: "Try again" on "Net Timeout Error" page loads wrong URL → "Try again" on XUL error page loads wrong thing after going back
Flags: blocking1.8b4? → blocking1.8b4-
Priority: -- → P2
also fails with DNS errors; changing URL, as those are faster to test ;)
Assignee: benjamin → cbiesinger
URL: http://1.2.3.4/ → http://www.invalid./
Priority: P2 → P1
hmm, the Reload button does the right thing...
ok, mOSHE is wrong and mLSHE is null.

I assume SH remembers a load type of ERROR_PAGE which makes InternalLoad not set
mLSHE...
Whiteboard: [ETA 8/6]
sigh, I was wrong... there is no mLSHE created for shistory traversal, so mOSHE
never gets a new value...
Attached patch patch (obsolete) (deleted) — — Splinter Review 
I hate docshell.
Attachment #191703 - Flags: superreview?(darin)
Attachment #191703 - Flags: review?(bryner)
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta4
(In reply to comment #9)
> sigh, I was wrong... there is no mLSHE created for shistory traversal, so mOSHE
> never gets a new value...

Hm, I'm not sure what you mean here.... are you referring to traversal via
bfcache?  We do set mLSHE in that case now (we didn't used to).
It's more complex than that... the thing is, EndPageLoad clears mLSHE, assuming
that something set mOSHE earlier. Usually, Embed seems to do that. But in this
case, the load resulted in an error, so Embed wasn't called. So mLSHE is cleared.

LoadErrorPage is then called, and it tries to set an mLSHE by calling OnNewURI,
but because this is a history load, it does not create a sh entry.

To work around that, I get the sh entry from the shistory object in this patch.
now, there's two things I noticed, namely:
- location.reload() and the reload toolbar button act differently (iirc one acts
on mOSHE, the other gets the entry from the shistory)
- mOSHE and mLSHE really seem redundant and could maybe be replaced with always
getting the entry from the nsISHistory object
bryner has a patch in the works to make mOSHE and the nsISHistory always match,
I believe.
No longer blocks: 302991
My patch (which landed) is more to cover the case where we navigate between
session history clones.  It doesn't do anything for this case.
Attachment #191703 - Flags: superreview?(darin) → superreview?(bzbarsky)
Attachment #191703 - Flags: superreview?(bzbarsky) → superreview+
Flags: blocking-aviary1.5+
*** Bug 316327 has been marked as a duplicate of this bug. ***
Comment on attachment 191703 [details] [diff] [review]
patch

Looks ok, maybe comment in particular on why mLSHE might be null.
Attachment #191703 - Flags: review?(bryner) → review+
Attached patch merged to trunk (deleted) — — Splinter Review 
with comment added.
Attachment #191703 - Attachment is obsolete: true
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.771; previous revision: 1.770
done
Checking in docshell/shistory/public/nsISHistory.idl;
/cvsroot/mozilla/docshell/shistory/public/nsISHistory.idl,v  <--  nsISHistory.idl
new revision: 1.15; previous revision: 1.14
done
Checking in docshell/shistory/src/nsSHistory.cpp;
/cvsroot/mozilla/docshell/shistory/src/nsSHistory.cpp,v  <--  nsSHistory.cpp
new revision: 1.77; previous revision: 1.76
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 212390 [details] [diff] [review]
merged to trunk

I think we should take this on the branch too.
Attachment #212390 - Flags: approval-branch-1.8.1?(bzbarsky)
hm, actually, the first attachment is the right one for the branch (shistory wasn't moved to docshell/ there)
Comment on attachment 212390 [details] [diff] [review]
merged to trunk

Can't change interfaces on the branch.  :(
Attachment #212390 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1-
As in, if we want this on branch we need to have a branch-only interface or something.
*** Bug 351906 has been marked as a duplicate of this bug. ***
*** Bug 352950 has been marked as a duplicate of this bug. ***
Depends on: 421067
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008031804 Minefield/3.0b5pre ID:2008031804
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: