299547 - Make sure fastback plays well with error pages

Status: RESOLVED FIXED

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

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

XUL Error Pages should work well even if fastback is enabled. I'm not sure what
the state is currently.

Comment 1

[Ben Basson]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

"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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Caleb]

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.

Comment 5

[Caleb]

(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.

Comment 6

[Brian Ryner (not reading)]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Caleb]

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.

Comment 9

[Pardal Freudenthal (:ShareBird)]

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...

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

comment 9 is bug 302115

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

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...

Comment 12

[Boris Zbarsky [:bzbarsky]]

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?

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 14

[Darin Fisher]

Comment on attachment 191737 [details] [diff] [review]
patch

sr=darin

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 191737 [details] [diff] [review]
patch

a=me+shaver for 1.8b4.

/be

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: merged to trunk

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.715; previous revision: 1.714
done