321778 - Dataloss on back after bfcache eviction

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  Current trunk Seamonkey build (can't test in a trunk Firefox for the time being).

STEPS TO REPRODUCE:

1)  Set the browser.sessionhistory.max_total_viewers preference to 1
2)  Load any bugzilla bug page (say this one).
3)  Type "aaaaa" in the keyword field on the bug page.  Type "test" in the
    comment textarea for good measure.
4)  Submit.  This should show an error page from bugzilla about this being an
    invalid keyword.
5)  Open a new window or tab and load about:, then about:blank in it, one after
    the other (so that the viewer that got saved in step 4 gets evicted).
6)  In the original window or tab we were testing with, hit the "back" button.

EXPECTED RESULTS:  The keyword field contains "aaaaa" and the comment textarea contains "test".

ACTUAL RESULTS: Both are empty.  The text is gone.

This bit me 3-4 times over the last two days, and I'm not even using bugzilla that much.  Interestingly, the state of radio buttons _is_ restored; just not the state of text inputs.

I'm marking this as a blocker because it makes bugzilla pretty painful to use...

Ria, Peter, would you possibly be able to find a regression range for this?  I Think I was already seeing the problem with a 2005-12-26 build, and I _think_ this problem didn't exist when I left on 2005-12-12, but no guarantees about either of those...

Comment 1

[Ria Klaassen (not reading all bugmail)]

Regression between 1.9a1_2005100709 and 1.9a1_2005100805.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-10-07+08%3A00%3A00&maxdate=2005-10-08+05%3A00%3A00&cvsroot=%2Fcvsroot

Comment 2

[Peter van der Woude [:Peter6]]

I'll try to verify this later today.

Comment 3

[Peter van der Woude [:Peter6]]

confirmed

works in firefox-1.6a1.en-US.win32 20051007 0608 pst
fails in firefox-1.6a1.en-US.win32_20051008_0659 pst

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&filetype=match&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20051007+0500&maxdate=20051008+0700&cvsroot=%2Fcvsroot

looks like bug 292962 / bug 311269 at 20051007 1320pst

Comment 4

[Peter van der Woude [:Peter6]]

bug 292962 / bug 311269 landed on MOZILLA_1_8_BRANCH on 20051018 1631pst
causing the regression there aswell.

Comment 5

[Peter van der Woude [:Peter6]]

windows sux.

after a reboot i wasn't able to reproduce the problem on branch at all.

Comment 6

[Peter van der Woude [:Peter6]]

I think you can remove the ? blocking1.8.1 and  ? blocking1.8.0.1 flags Boris.
Somehow my system must have been messed up.
I tried on 2 other systems and verify this doesn't affect branch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Good to hear that branch's ok.

I'm still not sure how we manage to only get confused about text controls; I guess I'll try to debug a bit when I get back if we haven't made any progress by then.

Comment 8

[Hideo Saito]

Please refer to posted patch(attachment 207342 [details]) to Bug 321671.

Comment 9

[Hideo Saito]

posted patch is attachment 207341 [details] [diff] [review].

Comment 10

[Hideo Saito]

Attachment: patch

I think that the problem of the loss of text on text control area should be fixed by fixing the Bug 321671, however, the problem still remains. Could you review this patch after Bug 32167 was fixed.

The cause of loss of added text on text control area is that added text is not
synchronized when closing and destruction of the DocumentViewer frame issue
from exceeding the number of cached viewers of session history.

Comment 11

[Hideo Saito]

(In reply to comment #10)

> Could you review this patch after Bug 32167 was fixed.

Could you review this patch after Bug 321671 was fixed.

Comment 12

[Brian Ryner (not reading)]

Ok, I stepped through this one.  As we destroy the content viewer, it calls nsDocument::Destroy(), which recursively calls UnbindFromTree() on the content tree.  For form control elements, this calls SaveState().  Because the document is marked as having a hidden pres shell, GetPrimaryFrameFor fails when called by GetFormControlFrameFor, so we don't find the form control frame for the text input.  As a result, the default value is persisted rather than the user-typed value.

Jonas, Boris, any thoughts on the best way to fix this? It's definitely a regression from bug 292962.  The patch here might be ok but it feels kind of fragile.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm...  So the two things I've thought of so far are:

1)  Mark the shells as not hidden before calling Destroy().  That way the text controls can find their frames and other such things that should happen on destruction will also work right.  Since the presshell is about to be destroyed anyway, I don't think this would be a problem.  That's effectively what the patch does, since DropPresentationState() calls mDocument->SetShellsHidden(PR_FALSE);

2)  Make all the text control frames in the presentation we're about to destroy call SetValueChanged() on their input nodes (so that the right value is stored in the input node).  I like this less, frankly.  It's just as fragile as any of the other solutions, and doesn't cover as many cases (only fixes text controls, not other things).

So I'd say either explicitly call mDocument->SetShellsHidden(PR_FALSE); or go with the patch here with a clear comment explaining why SyncPresentationState() needs to be called before Destroy() here (explicitly mentioning SetShellsHidden).

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I don't like 2) very much either. It makes sense to restore things fully before tearing it down since that means we'll excersice well tested codepaths. Weather to do that through an explicit call to SetShellsHidden or not I don't really have an oppinion. I think i'm pretty ok with the patch if it gets some better comments.

Comment 15

[Brian Ryner (not reading)]

Attachment: patch

better comment, and patch the other eviction site

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 207638 [details] [diff] [review]
patch

Excellent.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

While I think this patch is good in general, i just thought of an alternative solution. We could make the code that stores state ignore the fact that the presshells are hidden and enable it to grab the frame anyway. Either through an additional argument to GetFormControlFrame, or by temporarily enabling the presshells.

Do you think there's a need for that even with this patch?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I don't think there's a need for that with this patch...

Comment 19

[Brian Ryner (not reading)]

checked in