74639 - Going back to forms loses form data.

Status: VERIFIED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This is a consequence of bug 73490 being fixed.

If I fill out a form at say
http://bugzilla.mozilla.org/show_bug.cgi?id=73490, then click on "Vote for this
bug" and then hit "back", the form data should not be cleared....  Currently it
is (linux build 2001-04-02-05).

Comment 1

[gordon]

I think this is more of a history issue, rather than a cache issue.

Comment 2

[Akkana Peck]

Makes bugzilla hard to use -- I keep losing data because of mistyping someone's
email address or forgetting a component and getting the error page, and losing
my changes when I go back.

Comment 3

[Radha on family leave (not reading bugmail)]

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

Comment 4

[Eric Pollmann]

I'm seeing some strange behavior...  When I go to yahoo.com, type in a search
term, then click back two things happen.  First, the page lays out and the
history state is restored correctly, then the entire page goes blank, and all of
the history state is blanked out.  I don't know why the page is blanking, but
this may be another symptom of the underlying problem?

Comment 5

[Claudius Gayle]

platform all/all. dupe bug 72701 has bugzilla example.

radha, the dupe's target milestone was set to 0.9.1

Comment 6

[Brendan Eich [:brendan]]

I think we want to fix this for mozilla0.9.  Radha, can a fix be developed and
tested/reviewed in the next few days?  We'd take it into the frozen trunk or the
0.9 branch, once the trunk re-opens for 0.9.1.  Can you say more about what
broke and what's needed to fix it?

/be

Comment 7

[Radha on family leave (not reading bugmail)]

Looking at the bugzilla example, the layoutHistoryState is set on the new 
PresShell by docshell. The PresShell call returns success, This should take care 
of putting the form values back on the page. I don't see a reload or a 
re-rendering happening on the bugzilla page. 
Agree that this needs to be fixed  ASAP. Shall try to get it in for 0.9. 

Comment 8

[Radha on family leave (not reading bugmail)]

cc'ing jst, as I understand he may have some clue on what broke this feature. 
Bugzilla form elements are working fine on my 4/9 builds, but not on 4/16 builds

Comment 9

[Eric Pollmann]

Both yahoo.com and bugzilla pages have badly nested forms, which causes the form
and all of it's contents to be demoted from a container to a leaf element.

The frames for form controls (text inputs, ...) get destroyed and recreated,
which causes us to loose the information.  When they get recreated, the
information *should* be restored, but isn't for some reason.  (It is possible
that this never worked, but a recent change, e.g. improved cache, could have
made the problem more obvious and on more pages.  If the page comes in faster or
in larger chunks, form demotion could cause larger blocks to get demoted / more
state to be lost).

I'm also looking at this and will update the bug report if I can make heads or
tails of what is going on.

Comment 10

[Eric Pollmann]

Attachment: reduced test case

Comment 11

[Brendan Eich [:brendan]]

Can't help yahoo.com, but I'm cc'ing mozilla-staffers who can help ensure that a
bug against bugzilla for spewing unbalanced forms is on file.

/be

Comment 12

[Eric Pollmann]

Attachment: patch, capture frame state when content is being removed

Comment 13

[Eric Pollmann]

This patch fixes bugzilla, yahoo, and the reduced test case.  Do we also want to
put CaptureStateFor before the second DeletingFrameSubtree in
nsCSSFrameConstructor::ContentRemoved?

CC'ing Nisheeth for insight on the performance impact of this change.

Comment 14

[Eric Pollmann]

Attachment: patch 2, Capture state before all calls to DeletingFrameSubtree

Comment 15

[Eric Pollmann]

I did some clock-watch performance testing of this change.  In the bugzilla
query page, when a new component is selected, this code gets called a *lot*
(thousands of times).

Select "Browser component:               Before: 15 seconds   After: 15 seconds
Select "Browser Localization" component: Before:  9 seconds   After:  9 seconds

Seems that if it does have a performance impact, it is drowned out by the work
done to destroy and recreate the frames themselves.

Comment 16

[Eric Pollmann]

Turns out we can't use this patch because it may cause bad things to happen to
XUL tree widget performance.  I'm working on another patch that is specific to
SinkContext::DemoteContainer, which I'll attach as soon as I get it compiling...

Comment 17

[Radha on family leave (not reading bugmail)]

I think it is appropriate for pollmann to own this bug right now. 

Comment 18

[Eric Pollmann]

Accepting...

Comment 19

[Eric Pollmann]

Attachment: patch 3: save/restore frame state in demoteContainer

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Bug 76714 is the Bugzilla bug that exposes this problem, although I was unable
to consistently reproduce it with 2001-04-02-05 and 2001-04-18-08.  Sometimes my
form data was lost, sometimes it wasn't, and sometimes the data that appeared
was from the second-to-last time I filled out the form rather than the last time.

Comment 21

[Brendan Eich [:brendan]]

[e-mail] wrote:

> Sometimes my form data was lost, sometimes it wasn't, and sometimes the data
> that appeared was from the second-to-last time I filled out the form rather
> than the last time.

I've seen this last thing (sometimes the data is from the second-to-last time)
too, very rarely.  Is it a separate bug?

/be

Comment 22

[Eric Pollmann]

That last behavior is definitely a separate bug.  Radha, is one already filed on
that?

Comment 23

[Eric Pollmann]

Attachment: patch 4: Call recursive frame state capture/restore only once

Comment 24

[Eric Pollmann]

r=[e-mail], sr=[e-mail]

Ready to go, just waiting for approval.

Comment 25

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9

Comment 26

[Eric Pollmann]

Attachment: patch 5: Also fix problem where occasionally many controls were blanked out

Comment 27

[Eric Pollmann]

Patch 4 greatly exacerbates a problem where, occasionally, going to a page would
entirely blank out many or all controls.  (All radios deselected, list box
options deselected, first option of combo selected)

The problem turned out to be frames happily storing this blank state into the
LayoutHistoryState if they are requested to save their state before they are
initialized.  Patch 4 would cause this to happen quite frequently, as frames are
commonly not yet initialized when the form demotion happens.

The solution in patch 5 is to prevent this from happening by having frames *not*
save state until they are initialized.

Rerequesting r,sr,a

Comment 28

[Johnny Stenback (:jst)]

sr=jst, fixing bugzilla to not feed us with ill-nested tags would be a good
thing to do, is there a bug on that?

Comment 29

[Dave Miller [:justdave]]

Yes, there is.  Bug 76714.  It even has a patch on it already.  However, 
Bugzilla's in the middle of a release cycle right now so there's a freeze on 
checkins pending finalization of the docs.  So it may be a week or two before it 
gets checked in.

Comment 30

[Eric Pollmann]

Nisheeth says: r=[e-mail], just waiting for a=...

Comment 31

[Brendan Eich [:brendan]]

asa was ready to a= the earlier patch.  I'll a= this one, beating him to it, if
you coders and reviewers are all happy.  Get it in.

/be

Comment 32

[Eric Pollmann]

Fix checked in.  To verify:

1) Go to http://www.yahoo.com, enter a search term, press Enter, click back
button - your search term should still be there.

2) Open the reduced test case, enter a string.  Type another URL in the url bar,
click back - term should still be there.

-OR-

3) Go to bug query page or a bug report, make some changes, go somewhere else,
click back - the changes you made should still be there.

Thanks!

Comment 33

[Claudius Gayle]

this mostly works BUT try these steps:

Go to yahoo.com
1. enter a search term
2. now click any link, I clicked 'shopping'.
3. Click back --> no search term. This should work just like the bugzilla testcase.

For fun continue with these steps:
4. Click forward again.
5. Click back again - now the search button is blank? What up with that?

Continue with these steps or them instead of 4 and 5:
Alternate 4. type a search string.
Alternate 5. Click forward again
Alternate 6. Click back. Now your search string is the label on the search button instead of
'Search' like yahoo intended.

*I couldn't break the reduced testcase or the bugzilla case, but this bug is reopened just for
failing my steps 1,2, and 3. The other stuff is just freakish related behavior I felt I should
document since I figured out how to reproduce it.

Comment 34

[Keyser Sose]

Seeing what claudius is seeing as well.

Comment 35

[Eric Pollmann]

The blank button (and probably the blanked state on the same page) sounds like
an off-by-one error with the content ID.  This is a symptom of our whole system
of keying the session history state by the content ID being flawed.  It's a
known problem that's been around since forever.  I'll open a new bug on making
this smarter or "fuzzier" in some way.

In the meantime, since the majority case that was broken is now fixed,
re-marking this as fixed - will attach bugid of new bug...

Comment 36

[Eric Pollmann]

New issue is bug 77834.

Comment 37

[Claudius Gayle]

marking this VERIFIED Fixed based on my earlier testing since remaining issues are addressed
in a different bug (bug 77834).