Dataloss on back after bfcache eviction

RESOLVED FIXED

Status

()

Core
Document Navigation
--
blocker
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: bz, Assigned: Brian Ryner (not reading))

Tracking

({dataloss, qawanted, regression})

Trunk
x86
Windows XP
dataloss, qawanted, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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...
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
I'll try to verify this later today.
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
bug 292962 / bug 311269 landed on MOZILLA_1_8_BRANCH on 20051018 1631pst
causing the regression there aswell.
Keywords: regression
(Reporter)

Updated

12 years ago
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
windows sux.

after a reboot i wasn't able to reproduce the problem on branch at all.
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
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.
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?

Comment 8

12 years ago
Please refer to posted patch(attachment 207342 [details]) to Bug 321671.
Depends on: 321671

Comment 9

12 years ago
posted patch is attachment 207341 [details] [diff] [review].

Comment 10

12 years ago
Created attachment 207577 [details] [diff] [review]
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.
Attachment #207577 - Flags: review?(bryner)

Comment 11

12 years ago
(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.
(Assignee)

Comment 12

12 years ago
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.
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).
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.
(Assignee)

Comment 15

12 years ago
Created attachment 207638 [details] [diff] [review]
patch

better comment, and patch the other eviction site
Assignee: nobody → bryner
Attachment #207577 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #207638 - Flags: review?(bzbarsky)
Attachment #207577 - Flags: review?(bryner)
Comment on attachment 207638 [details] [diff] [review]
patch

Excellent.
Attachment #207638 - Flags: review?(bzbarsky) → review+
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?
I don't think there's a need for that with this patch...
(Assignee)

Comment 19

12 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Updated

11 years ago
Flags: blocking1.9a1?

Updated

9 years ago
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
You need to log in before you can comment on or make changes to this bug.