77834 - Form data corrupted on going forward/back.

Status: VERIFIED FIXED

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

Description

[Eric Pollmann]

There is a problem that sometimes when a page has it's state restored, the
content ID's of the page will have changed by one (or a few?) when the page
comes back the second time.  This causes us to miss on restoring frame state, or
even worse, if the adjacent control is of the same type, we can restore the
state into the wrong control!

Two solve this problem we need:
1) More specific keys (key by the name attribute? and id attribute?  Unique
types for hidden and button state?)

2) Fuzzier restoration (if this piece of content does not match by id, search
plus or minus a few nodes to find a match?)

Any suggestions or comments welcome!

Comment 1

[Eric Pollmann]

Probably overloading myself for 0.9.1, subject to pushing...

Comment 2

[Eric Pollmann]

See bug 74639, which this bug was split out of.

Comment 3

[Eric Pollmann]

I tried  this simple approach with limited success in my tree.  In addition to
making us store more information and work harder when restoring state, it did
not fix the problem with yahoo.com.  :(  There were, however, some number of
cases that this did work where we would have lost data before, so we could be on
the right track...

Comment 4

[Eric Pollmann]

Attachment: hack to do fuzzy matching (inefficient, doesn't fix problem)

Comment 5

[Kevin McCluskey (gone)]

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

Comment 6

[Radha on family leave (not reading bugmail)]

Eric, I noticed in yesterday's build that the form values just don't get 
restored sometimes, even though the PresShell is presented a LHS by SH. I think 
the trend  I noticed was, it doesn't work the first time the historystate is 
restored and somewhat works in future restorations. Does this bug address this 
problem too?

Comment 7

[Eric Pollmann]

Please consider this bug for 0.9.1 blocker status.  The current problems with
session history state capture/restore sometimes result in form corruption.  For
example, on the main search page for yahoo.com, if you type in a search term,
then go somewhere else and click back, the text that should be restored to the
search input field is sometimes shifting over and showing up on the button instead.

This "shifting by one" of the form data could happen on any form, and would be a
real bummer for an end user on, say, an internet banking site.  While this does
not happen every time or on every site, even an occasional mistake is not
acceptable.

Another problem that is sometimes seen is that state will not be restored at all
because the content id's in the document shifted by one and there is no control
there to restore the state to.  This is simple data loss when going back/forward
and is also fairly annoying.

I'm testing a medium-sized patch that addresses this problem - should have it
ready soon, and will attach it to this bug report as soon as I verify it is not
causing bad things to happen.

Comment 8

[Eric Pollmann]

Attachment: patch 2 - use more robust key for history state (requires next file)

Comment 9

[Eric Pollmann]

Attachment: new file content/base/public/nsIContentList.h

Comment 10

[Eric Pollmann]

Attachment: patch 3 - includes fix for msn.com edge case (<style> causing out of date content lists)

Comment 11

[Phil Peterson]

Eric, are you getting reviews for this?

Comment 12

[Eric Pollmann]

The fix for this bug causes a reproducible approximately 2% slowdown in the page
load performance tests.  (Tests were run many times since this is so close to
the "noise" limit for jrgm's page load tests)  I'm working on a few
optimizations, but none are very promising.

The fix has been sr'd by jst, but on the condition that the performance issues
be addressed as soon as possible (after 0.9.1).  I'll get a review today.

Comment 13

[Eric Pollmann]

Attachment: patch 4: cleanups suggested in sr=jst and r=nisheeth (and harishd)

Comment 14

[Eric Pollmann]

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

This patch is ready to check in.  There is an approximate 2% slowdown in page
load times across the board due to this patch, but it fixes a critical dataloss
bug that balances the cost.  There is a good chance that this performance hit
can be reduced or eliminated by further optimization work during the 0.9.2
milestone.

Comment 15

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1

Comment 16

[Asa Dotzler [:asa]]

please disregard above comment. sorry for the confusion and the spam.

Comment 17

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1

Get it in.

Comment 18

[Phil Peterson]

Big patch. What should QA be doing to vet this? Also, nisheeth or jst, any ideas
on the slowdown? 

Comment 19

[Eric Pollmann]

Fix checked in.

There is no 100% way to verify this, but try going to yahoo.com, and typing
something in the search box, then clicking on a link on the page (e.g. Travel),
then click back, forward, back, forward, ... The text in the search box should
not magically disappear, nor should it appear on the button next to the search box.

Things QA should also test (I tested these before checking in, but there might
be related areas...) The addressing pane in the mail compose window relies upon
session history to store addresses that are scrolled offscreen. Make sure
sending mail to more than three people reaches all of it's intended recipients.
(I tested this, and caught a bug early on, it works now)

Test long pages with forms at the end (e.g., the Find Bug # form at the bottom
of this bug report.) Enter something in that form, go forward, back, ... The
text you entered should not disappear.

Make sure every *type* of 'stateful frame' retains state (radios, checkboxes,
text inputs, passwords, textareas, selects, isindex, scroll position)

As for the performance hit, the majority of that is due to an additional flush
added to the content sink. In addition to making the new session history key be
generated more reliably, this flush also fixes a few quirks where javascript
could be made to return the wrong results, or fail to execute. I can't really
think of a way to get rid of this, but anything that would reduce the frequency
of this flush would surely improve the ~2% hit we see.

Also, a contributor to the performance degradation is the form controls content
list that I added to the frame manager. I plan on getting rid of this for 0.9.2
if possible, which should help a bit. Most other parts of this patch should
have negligable performance impact (fingers crossed)  :)

Comment 20

[Claudius Gayle]

VERIFIED Fixed. yes I checked all the crazy scenarios