form data only remembered for first form on page

RESOLVED FIXED in mozilla1.0

Status

()

P1
major
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: jruderman, Assigned: john)

Tracking

({dataloss, regression})

Trunk
mozilla1.0
dataloss, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1][m5+][eta 4/29][FIX])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

17 years ago
Form data is only remembered on back/forward for the first form in a page.

I noticed this bug at http://www.cs.hmc.edu/~jruderma/s/, but it also happens at
http://www.plastic.com/users.pl (if you're not logged in).

0314   - wfm
032903 - wfm
033109 - broken
040303 - broken
0419   - broken
(Reporter)

Comment 1

17 years ago
Created attachment 80239 [details]
testcase

Steps to reproduce:
1. Fill in each form element, or just the last two.
2. Leave the page by submitting one of the forms or hitting the back button.
3. Return to the page.

Result: what you entered in the second form is gone.
(Reporter)

Updated

17 years ago
Keywords: dataloss, regression

Comment 2

17 years ago
I am seeing this on linux as well trunk 2002042015

os -> all
OS: Windows 98 → All
(Reporter)

Comment 3

17 years ago
Bug 134700 comment 3: also happens at
http://www.mozilla.org/quality/help/bugzilla-helper.html, which has a search
form at the top.
(Reporter)

Comment 4

17 years ago
*** Bug 134700 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 5

17 years ago
Hmmm.  We do some shenanigans in nsFrameManager::GenerateStateKey().  That may
be the cause.  Regardless, this is almost certainly me, and probably caused by
the bug 108309 checkin.

Nominating for nsbeta1
Assignee: radha → jkeiser
Keywords: nsbeta1

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+
Priority: -- → P1
Whiteboard: [adt1]
Target Milestone: --- → mozilla1.0

Updated

17 years ago
Whiteboard: [adt1] → [adt1][m5+]
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

17 years ago
*** Bug 140245 has been marked as a duplicate of this bug. ***

Comment 7

17 years ago
John, do you have an ETA on when you think you might have a patch for this?
(Assignee)

Comment 8

17 years ago
I can have a patch for you right now that fixes the problem and is low-risk, but
carries a 4-5% pageload hit.  I will hopefully have a fix that does not take a
pageload hit and still fixes the problem by Monday, but it will have higher
risk.  Let me know if you want the low-risk low-performance patch and I'll post it.
*** Bug 140697 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 10

17 years ago
BTW, I have thought up a low-risk way to do this for branch (I want to do it The
Right Way for trunk).  Specifically, save the current form #, or even
immediately update the document.forms list (which would work better for
dynamically added forms, which we barely support anyway).  Patch tomorrow once I
get away from meetings and such.

Comment 11

17 years ago
changing eta based on conversation with John.
Whiteboard: [adt1][m5+] → [adt1][m5+][eta 4/29]
(Assignee)

Comment 12

17 years ago
Created attachment 81631 [details] [diff] [review]
Patch

The problem is that while we are parsing, document.forms[] has not been updated
with all the forms yet.  So when we try to restore, and it has not been updated
yet, (the intent was) we generate the state key assuming that the form
currently being parsed is the one the control is being added to.  The way we
did this hack was wrong, however, because when multiple forms are on the page,
and *neither* has been placed in document.forms, we will use the index of the
first form for controls in the second form.

Calling FlushPendingNotifications(), which populates document.forms and does a
lot of other stuff besides (like creating frames), fixes the problem but
carries a 4-5% pageload penalty.

This patch, which is WALLPAPER to allow us to work in 1.0 without the
performance problems of FlushPendingNotifications or the high risk of the real
fix.

The real fix is to make FlushPendingNotifications not take so darn long (there
are several ways of going about this) or even make certain content state update
*immediately*.
(Assignee)

Comment 13

17 years ago
Created attachment 81632 [details] [diff] [review]
Patch (diff-u)

diff -u, same patch
Attachment #81631 - Attachment is obsolete: true

Comment 14

17 years ago
Comment on attachment 81632 [details] [diff] [review]
Patch (diff-u)

r=rods

Updated

17 years ago
Attachment #81632 - Flags: review+
(Assignee)

Comment 15

17 years ago
Patch is awaiting sr=, a= and adt1.0.0+, all three mails sent, it's OK to hold 
your breath :)  Won't happen until tomorrow morning though.  
Keywords: adt1.0.0
(Assignee)

Updated

17 years ago
Whiteboard: [adt1][m5+][eta 4/29] → [adt1][m5+][eta 4/29][FIX]

Comment 16

17 years ago
Comment on attachment 81632 [details] [diff] [review]
Patch (diff-u)

sr=attinasi for the temporary patch. Actually, I like it better than the 'real'
fix :) But then, I detest FlushPendingNotifications, and look forward to your
ideas for making it faster...
Attachment #81632 - Flags: superreview+

Comment 17

17 years ago
adding adt1.0.0+.  Please check this in as soon as possible and add the
fixed1.0.0 keyword after getting drivers approval.
Keywords: adt1.0.0 → adt1.0.0+
Comment on attachment 81632 [details] [diff] [review]
Patch (diff-u)

a=rjesup@wgate.com, please check into branch and trunk.
Attachment #81632 - Flags: approval+
(Assignee)

Comment 19

17 years ago
Checked in to branch only.  I'll ask Jesup what's up with the checkin to trunk
thing.
Keywords: fixed1.0.0
Moving nsbeta1+ to nsbeta1- to get it off the nsbeta1 radar since a patch has
already been checked into the 1.0.0 branch. This bug is left open to work on a
fix for the trunk. 
Keywords: nsbeta1+ → nsbeta1-
(Assignee)

Updated

17 years ago
Depends on: 144072

Comment 21

17 years ago
Is somebody moving towards getting this fixed on the trunk?  It's a very
annoying bug.  While everybody's going on and on, ad nauseam, in Bug 46845 about
whether or not form fields should reset upon page reload, this much more
impactful data loss problem is being allowed to persist.

Updated

17 years ago
Keywords: mozilla1.0.1
Hardware: PC → All
(Assignee)

Comment 22

17 years ago
xah, are you not seeing this in 1.0?  It should already be fixed there.
(Reporter)

Comment 23

17 years ago
See also bug 58724, "should autofill password while page is loading, not
onload".  Could the same fix work for both bugs?

Updated

17 years ago
Keywords: mozilla1.0.1 → mozilla1.1
(Assignee)

Comment 24

16 years ago
Same fix won't fix both bugs, but it has become apparent that the
FlushPendingNotifications thing is going to take longer than anticipated, so I'm
going after approval for this patch for 1.1.

Comment 25

16 years ago
a=asa (on behalf of drivers) for checkin to 1.1
(Assignee)

Comment 26

16 years ago
Checked in to 1.1beta.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 27

16 years ago
Is it in the trunk as well?
(Assignee)

Comment 28

16 years ago
That's trunk.

Updated

10 years ago
Component: History: Session → Document Navigation
QA Contact: claudius → docshell
You need to log in before you can comment on or make changes to this bug.