Closed Bug 299547 Opened 19 years ago Closed 19 years ago

Make sure fastback plays well with error pages

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta4

People

(Reporter: Biesinger, Assigned: Biesinger)

References

Details

(Keywords: dataloss, Whiteboard: [no l10n impact])

Attachments

(1 file, 1 obsolete file)

XUL Error Pages should work well even if fastback is enabled. I'm not sure what
the state is currently.
Not sure how you define "work well", but I don't see any problems with the XUL
Error Pages as-is (apart from still being ugly). Should this depend on bug 280190?
"work well" means that form data should end up in the right session history
entries, that loads on the previous page should correctly affect whether
fastback kicks in, and so forth.  The key here is that error pages do an
InternalLoad from within the error handling.
For example, bug 299008 comment 17 describes a case in which bfcache loses form
data when an error page is shown and then you go back to the previous page...
As much as I'd like to make this bug block 1.8b3, I think it's a bit too late.

This bug basically brings back the behaviour described in bug 299008 comment 0
when bfcache is on, and it also seems to reget the previous page once you press
the back button from the error page.

I'm asking for 1.8b4 blocking in this case.
Flags: blocking1.8b4?
(In reply to comment #4)
> This bug basically brings back the behaviour described in bug 299008 comment 0
> when bfcache is on, and it also seems to reget the previous page once you press
> the back button from the error page.

Correction: I meant that the patch in bug 299008 is only half of the fix, and
since bfcache is now on by default, the issue described in bug 299008 comment 0
has resurfaced.

Sorry for the spam.
Whiteboard: [no l10n impact]
Currently, it looks like we don't end up with the page in bfcache if the next
navigation is to a XUL error page.  Definitely needs fixed, but I don't think
it's a 1.5 blocker.
Target Milestone: --- → mozilla1.9alpha
There's got to be more to it than that, since bug 299008 is fixed when bfcache
is off but still around when bfcache is on... and that's a dataloss bug. 
Setting severity to critical based on that.
Severity: normal → critical
Keywords: dataloss
Yes, I don't think it's a good idea pushing this to 1.9.

Not only that bfcache is disabled on the error page, going back from it resets
any form fields.
Flags: blocking1.8b4? → blocking1.8b4+
Assignee: adamlock → bryner
Target Milestone: mozilla1.9alpha → mozilla1.8beta4
I'm not really sure if is the same bug, but I have some steps to reproduce the
behaviour I considere a bug:

1.Open the firefox start page (for me is Google)
2.Type anything like http://www.gfgtrghfuuf.com/ to call the error page.
3.Press "Try Again". The error page will load again
4.Now press the back button and after this the forward button
5.The error page will appear, but if you press the "Try Again" buttom it will
load the start page...
Assignee: bryner → cbiesinger
Priority: -- → P1
Attached patch patch (obsolete) — Splinter Review
I think this should be all that's needed. All the error page load stuff is done
at the end of EndPageLoad.

I am not sure what the purpose of the load type check in EndPageLoad is... it
seems to check the type of the next load...
Attachment #191737 - Flags: superreview?(darin)
Attachment #191737 - Flags: review?(bryner)
Whoa.  Why does that help?  What if this method returns false for some other
reason (eg there's an active network request)?  Would that cause the same issues
with form state as returning false because it's an error page load causes now? 
For that matter, why does the false return when error pages are disabled not
cause issues?

Or is it just the second hunk that's relevant here?
Whiteboard: [no l10n impact] → [no l10n impact] [ETA 08/07]
oh, I should note... the form state issue is no longer an issue. it must have
been fixed by something else (Caleb confirmed that). So all that patch is
addressing is the fact that the presentation wasn't actually cached.
Attachment #191737 - Flags: review?(bryner) → review+
Status: NEW → ASSIGNED
Whiteboard: [no l10n impact] [ETA 08/07] → [no l10n impact] [1.8 Branch ETA 08/07]
Whiteboard: [no l10n impact] [1.8 Branch ETA 08/07] → [no l10n impact] [1.8 Branch ETA 08/10]
Comment on attachment 191737 [details] [diff] [review]
patch

sr=darin
Attachment #191737 - Flags: superreview?(darin) → superreview+
Comment on attachment 191737 [details] [diff] [review]
patch

a=me+shaver for 1.8b4.

/be
Attachment #191737 - Flags: approval1.8b4? → approval1.8b4+
Attached patch merged to trunkSplinter Review
Attachment #191737 - Attachment is obsolete: true
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.715; previous revision: 1.714
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [no l10n impact] [1.8 Branch ETA 08/10] → [no l10n impact]
You need to log in before you can comment on or make changes to this bug.