138892 - form data only remembered for first form on page

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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

Comment 1

[Jesse Ruderman]

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

Comment 2

[Vadim Berezniker]

I am seeing this on linux as well trunk 2002042015

os -> all

Comment 3

[Jesse Ruderman]

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

Comment 4

[Jesse Ruderman]

*** Bug 134700 has been marked as a duplicate of this bug. ***

Comment 5

[John Keiser (jkeiser)]

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

Comment 6

[John Keiser (jkeiser)]

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

Comment 7

[scottputterman]

John, do you have an ETA on when you think you might have a patch for this?

Comment 8

[John Keiser (jkeiser)]

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.

Comment 9

[Kai Lahmann (is there, where MNG is)]

*** Bug 140697 has been marked as a duplicate of this bug. ***

Comment 10

[John Keiser (jkeiser)]

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

[scottputterman]

changing eta based on conversation with John.

Comment 12

[John Keiser (jkeiser)]

Attachment: 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*.

Comment 13

[John Keiser (jkeiser)]

Attachment: Patch (diff-u)

diff -u, same patch

Comment 14

[rods (gone)]

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

r=rods

Comment 15

[John Keiser (jkeiser)]

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.  

Comment 16

[Marc Attinasi]

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

Comment 17

[scottputterman]

adding adt1.0.0+.  Please check this in as soon as possible and add the
fixed1.0.0 keyword after getting drivers approval.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

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

a=[e-mail], please check into branch and trunk.

Comment 19

[John Keiser (jkeiser)]

Checked in to branch only.  I'll ask Jesup what's up with the checkin to trunk
thing.

Comment 20

[Kevin McCluskey (gone)]

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. 

Comment 21

[Dan Tobias]

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.

Comment 22

[John Keiser (jkeiser)]

xah, are you not seeing this in 1.0?  It should already be fixed there.

Comment 23

[Jesse Ruderman]

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

Comment 24

[John Keiser (jkeiser)]

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

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 1.1

Comment 26

[John Keiser (jkeiser)]

Checked in to 1.1beta.

Comment 27

[Dan Tobias]

Is it in the trunk as well?

Comment 28

[John Keiser (jkeiser)]

That's trunk.