294258 - Reload clears form values, incorrectly

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  today's trunk build

STEPS TO REPRODUCE:

1)  Load this bug page
2)  Type something in the "Additional comments" area
3)  Click the "reload" button

ACTUAL RESULTS:  Text in "Additional comments" disappears

EXPECTED RESULTS:  Text doesn't disappear

NOTE:  Doing a second reload makes the text reappear.

This is a regression from bug 293588, of sorts.  The problem is that we're not
saving the form state until the new page gets painting unsuppressed, at which
point it's far too late to restore it into the new page (which we want to be
doing, since the session history entry is the same).

I _think_ we might be able to fix this by restoring the "walk over the whole
tree in SetScriptGlobalObject" code..  That'd mean two walks over the DOM tree
at document teardown, and saving state twice for all form controls (once in that
walk, once in the destructor).  I'm also not sure what edge cases would remain
then...

The other option (the one brendan suggested on IRC the other day) is to
condition the teardown behavior on the fastback pref so that we have the
pre-fastback-landing behavior when fastback is off and can sort out the
"fastback on" bugs.  I suspect that will just mean that we don't catch bugs that
appear only when the fastback cache entry is evicted, so I'm not very happy with
this approach.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is a pretty serious usability issue, what with sites calling reload()
randomly at times.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Note that bug 46845 is about ensuring that we *don't* save form data after
reloading. IMO that bug is invalid, especially since the current (intentional?)
behavior has been around for so long and people have come to expect it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I should clarify that I'm fine with having two different teardown codepaths as
long as we're all clear on the fact that this means the fastback one will get
pretty much no testing.

Comment 4

[Brendan Eich [:brendan]]

Another one to fix for 1.8b2/aviary1.1a1.

/be

Comment 5

[Brendan Eich [:brendan]]

Re: comment 2: since Netscape 1, if not NCSA Mosaic (history buffs feel free to
add details here), reload has not cleard saved form data -- only shift-reload. 
I'll comment in bug 46845.

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #3)
> I should clarify that I'm fine with having two different teardown codepaths as
> long as we're all clear on the fact that this means the fastback one will get
> pretty much no testing.


I wouldn't say that -- lots of people run with the pref, and will.  Not on the
same decimal order as without, but still enough to test the code paths (assuming
bugs are diagnosed and reported well!).

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

The "regular" teardown code and SH code is not well-exercised with the pref on,
since most history navigation uses fastback.  So users with the pref _on_
probably won't catch issues that only crop up once the fastback cache is evicted.

Comment 8

[Daniel C]

Attachment: Testcase.

Testcase.

1. Load up the page and change every field.
2. Reload the page.
3. Repeat 1.
4. Hard reload.

After 2 and 4 all elements are reverted to their original value.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050515
Firefox/1.0+

1.0.4 does the same thing.

Internet Explorer DOESN'T reset the fields, except for "password" and "file".

In my opinion, data should be retained (as the summary suggests.) However,
there is the issue that if the form has no <input type="reset"/> then it isn't
easy to start over.

I don't see why "password" should be exempt from this behaviour.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 1.0.4 does the same thing.

Then that's not a testcase for THIS bug, which is a trunk-only regression.

Comment 10

[jag (Peter Annema)]

DJC, the problem with your sample is unrelated to this bug, though the symptoms
seem the same. If I turn your XHTML sample into "transitional" HTML and add
<form> then the form data is retained on reload and cleared on hard reload as
normal in my Mozilla 1.8a5 build. You might be able to do the same for your XHTML.

Comment 11

[Timwi]

> However, there is the issue that if the form has no <input type="reset"/>
> then it isn't easy to start over.

Shift-Reload?

Comment 12

[Brian Ryner (not reading)]

So basically we'd just back out part of 293588.  That seems like the best
solution to me at the moment, given that (a) we need to have the form control
state saved _before_ the new page starts to load, and (b) we don't want to
UnbindFromTree until we're ready to destroy the old document.

I guess another idea is to special case reload so that we call UnbindFromTree()
when the old docviewer is Close()d; we know that's safe since the presentation
would never be cached.  In other words, make sure to set the history entry on
the docviewer _before_ calling Close() (maybe actually make it a parameter to
close), then call UnbindFromTree there if the history entry is null.  That
essentially restores the pre-fastback behavior when fastback is disabled.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.. I like this last idea.

Comment 14

[Brian Ryner (not reading)]

Attachment: patch

Call document->Destroy() from DocumentViewer::Close() if the document is not
going into session history.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

r=me (modulo unrelated cache and pwdmanager changes ;-)

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Though i'd really like if bz could have a look at this stuff too.

Comment 17

[chris hofmann]

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

bz, can you look at this as soon as possible?	one of the last on the way to
getting a alpha1 release candidate out for some feedback....

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

>Index: layout/base/nsDocumentViewer.cpp
>@@ -1253,6 +1255,9 @@ DocumentViewerImpl::Close()
>       mDocument->SetScriptGlobalObject(nsnull);
>     }
> 
>+  if (!mSHEntry)
>+    mDocument->Destroy();
>+

With more context, won't this tear down the DOM while we're trying to print it
in some cases?	That doesn't seem desirable...	That is, it seems like this
code should be in the same |else| as the SetScriptGlobalObject(nsnull) and that
we should have a Destroy() wherever it is we currently deal with the printing
thing finishing.

sr=bzbarsky with that fixed up.

Comment 19

[Frank Wein [:mcsmurf]]

This patch was checked in, jag checked in a fix for the red on btek (see
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/docshell/base&command=DIFF_FRAMESET&file=nsDocShell.cpp&rev1=1.682&rev2=1.683&root=/cvsroot).
 This patch also seems to have caused a small Tp regression (if it was this fix
and not vlad's one; or was it already clear that this fix would cause this Tp
regression?), the Tp went up from ~816ms to ~825ms.

Comment 20

[Boris Zbarsky [:bzbarsky]]

The Tp change is very odd-looking to me -- note that on luna it did build both
versions of the patch (with and without jag's fix) and the Tp jumped only when
jag fixed the build bustage.

Comment 21

[jag (Peter Annema)]

Tp on Luna actually did go down a little bit, but not completely. It's
unfortunate the bustage wasn't fixed earlier so we'd know whether it was the
patch or the same thing that caused Luna to go up.

Comment 22

[Boris Zbarsky [:bzbarsky]]

This is fixed (for 1.8b2).

Comment 23

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

This patch also adds a .reload parameter to doPageNavigation() in the docshell sub-harness.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 387507 [details] [diff] [review]
mochitest test case

Looks great!

Comment 25

[Boris Zbarsky [:bzbarsky]]

Pushed test as http://hg.mozilla.org/mozilla-central/rev/2d35f8343260