Closed Bug 138892 Opened 20 years ago Closed 20 years ago

form data only remembered for first form on page

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: jruderman, Assigned: john)

References

Details

(Keywords: dataloss, regression, Whiteboard: [adt1][m5+][eta 4/29][FIX])

Attachments

(2 files, 1 obsolete file)

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
Attached file 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.
Keywords: dataloss, regression
I am seeing this on linux as well trunk 2002042015

os -> all
OS: Windows 98 → All
Bug 134700 comment 3: also happens at
http://www.mozilla.org/quality/help/bugzilla-helper.html, which has a search
form at the top.
*** Bug 134700 has been marked as a duplicate of this bug. ***
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
Keywords: nsbeta1nsbeta1+
Priority: -- → P1
Whiteboard: [adt1]
Target Milestone: --- → mozilla1.0
Whiteboard: [adt1] → [adt1][m5+]
Status: NEW → ASSIGNED
*** Bug 140245 has been marked as a duplicate of this bug. ***
John, do you have an ETA on when you think you might have a patch for this?
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. ***
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.
changing eta based on conversation with John.
Whiteboard: [adt1][m5+] → [adt1][m5+][eta 4/29]
Attached patch Patch (obsolete) — Splinter Review
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*.
Attached patch Patch (diff-u)Splinter Review
diff -u, same patch
Attachment #81631 - Attachment is obsolete: true
Comment on attachment 81632 [details] [diff] [review]
Patch (diff-u)

r=rods
Attachment #81632 - Flags: review+
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
Whiteboard: [adt1][m5+][eta 4/29] → [adt1][m5+][eta 4/29][FIX]
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+
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.0adt1.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+
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-
Depends on: 144072
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.
Keywords: mozilla1.0.1
Hardware: PC → All
xah, are you not seeing this in 1.0?  It should already be fixed there.
See also bug 58724, "should autofill password while page is loading, not
onload".  Could the same fix work for both bugs?
Keywords: mozilla1.0.1mozilla1.1
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.
a=asa (on behalf of drivers) for checkin to 1.1
Checked in to 1.1beta.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Is it in the trunk as well?
That's trunk.
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.