346337 - Form input field data not retained on session restore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Tracy Walker [:tracy]]

seen with Bon Echo build 2006072803 on Windows XP


   1. Start Firefox
   2. Load test form : http://bclary.com/projects/form-utils/form.pl?method=post
   3. Note counter, time
   4. Modify inputs  (I put test in each field and add label 3 for the rest of the fields)
   5. Kill Firefox
   6. Start Firefox
   7. Restore Session

tested reuslts:  all of the select, checkbox and radio fields were resest to label 1.  The text fields were randomly restored.  Some not restored

expected results the text fields restored to "test" and the other inputs restored to label 3

Comment 1

[Tracy Walker [:tracy]]

was also seeing on ssl test pages.  

Comment 2

[Simon Bünzli]

That's less of a bug and more of a (quite valid) RFE since complete form data saving's never been implemented. Currently only text data is restored for non-encrypted pages -- and even that only as long as the text fields have an ID or a unique name - which however is mostly the case anyway. Unfortunately I've never managed to come up with a reasonable plan for how to cleanly implement this with decent resource usage...

Comment 7

[Michael Kraft [:morac]]

Attachment: Form element test case

I've found that the only form data that SessionStore will save is "text" type input fields and "textarea" elements.  Nothing else is saved.  This includes input element types of checkbox and radio as well as selection elements.

In addition if the text field either has no name or a name with spaces (not valid per W3 specifications) then those elements are not restored either.

The attached test case contains all basic types of form elements.

Comment 8

[Simon Bünzli]

Attachment: patch

This patch pretty much replaces all current form data handling code: We now save form data per frame and only access it while we're updating our data collection, anyway. OTOH we store data for all form elements, whether they've been touched or not (thus fixing bug 391814). Our internal data collection will later on be updated whenever a form element is modified (onChange and onInput) or whenever the user navigates around.

Form elements with IDs continue to be accessed through getElementById - all others are now accessed through XPath (which also fixes bug 367387). Since bug 319768 never made it into our main tree (or at least not into mozilla-central), I've ported the required code over to nsSessionStore and made it slightly more flexible (we walk to the form elements from the first parent node with an ID).

Finally, this also fixes the issue with input element names containing spaces, etc., so that the test form in attachment #328502 [details] is now completely restored.

Comment 9

[Simon Bünzli]

Attachment: don't save hidden fields

Mostly the same patch, except that we no longer save the values of hidden fields (which aren't user editable, anyway) and fix an issue with <input>s without having a type attribute set at all (those default to type="text").

Comment 10

[Simon Bünzli]

Attachment: don't dispatch change events

When restoring a form element, sometimes the dispatched change events can have unexpected side-effects. I've already several times almost changed bugs to fixed because of the change event to the resolution selection. So, better safe than sorry, I prefer not having those events for the time being.

A second change in this patch is that the generated XPath queries now take <input> names instead of types into account. That should make them slightly more reliable and easier to debug.

Note to self: We're still missing <input type="file">.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 334156 [details] [diff] [review]
don't dispatch change events

>     function restoreTextDataAndScrolling(aContent, aData, aPrefix) {
>       restoreTextData(aContent, aPrefix);
>+      if (aData.formdata)
>+        restoreFormData(aContent.document, aData.formdata);
      if (aData.children && aData.children[i]) {

if aData.formdata exists, is there a reason to try to restore legacy data? wouldn't legacy data no longer be present in the file after the first save w/ this patch?

>+var XPathHelper = {
>+  // these two hashes should be kept in sync
>+  namespaceURIs:     { "xhtml": "http://www.w3.org/1999/xhtml" },
>+  namespacePrefixes: { "http://www.w3.org/1999/xhtml": "xhtml" },
>+

this should probably be a js module, so others may use it.

Comment 12

[Simon Bünzli]

(In reply to comment #11)
> if aData.formdata exists, is there a reason to try to restore legacy data?

I was considering extension authors used to the old serialization. But then again, they really should migrate anyway... I'll change that.

> >+var XPathHelper = {
> 
> this should probably be a js module, so others may use it.

This code is currently quite specific to this use case (as in: we don't care if a form has moved in the DOM as long as the IDs and names still match; as opposed to: I want this node only if it hasn't moved at all). Generalizing it into a JSM won't thus be as straight-forward as in the JSON case.

So I'd really prefer this to go in for now (also considering that a JSM might not make Firefox 3.1 while this patch should) and then discuss a possible generalization later - although AFAICT we've currently got no other code in /browser or /toolkit generating XPath queries for given nodes at all, so it wouldn't be clear what use cases to generalize for...

Comment 13

[Simon Bünzli]

Attachment: including tests

... and finally also restoring <input type="file">.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 335175 [details] [diff] [review]
including tests

r=me, thanks

Comment 15

[Simon Bünzli]

Attachment: for check-in

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/5f754cb1370f

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now we have a Litmus test which checks the form data restoration after a crash. This is https://litmus.mozilla.org/show_test.cgi?id=6224

Simon, what does the automated test cover? Do we still need the Litmus test?

Comment 19

[Simon Bünzli]

The automated test covers saving and restoring form data when closing/duplicating tabs. These are however the same code paths as used for the crash case, so we should be fine without the Litmus test.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Simon. For now I have disabled the test. I'll mark it as in-litmus? so we can find a final decision when reworking this subgroup.

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848

Comment 21

[u279076]

in-litmus+
See tests 6224 to 6229.  They all check Form restore.

Comment 22

[Calc-Yolatuh]

Is there an implemented throw for the case of a target page having a changed layout since last load? Especially pages with incomplete or missing IDs on form elements. If a page without IDs is 'fixed' so that every field has a solid ID, or a single field added, removed, or altered (in name or type), how does XPath cope with this?

Disregarding speed, is there any fail on accuracy when XPath re-walks a form without explicit ID? Is it possible for field data to be displaced into a different field on restore?

Comment 23

[Teodosia Pop]

I am able to reproduce this issue using Mozilla/5.0 (Windows NT 6.1; rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre.

Comment 24

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #23)
> I am able to reproduce this issue using Mozilla/5.0 (Windows NT 6.1;
> rv:2.0b9pre) Gecko/20110109 Firefox/4.0b9pre.

If that's the case, please file a new bug with as much information as possible. This bug was fixed long ago.