Closed
Bug 321778
Opened 19 years ago
Closed 19 years ago
Dataloss on back after bfcache eviction
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: bryner)
References
Details
(Keywords: dataloss, qawanted, regression)
Attachments
(1 file, 1 obsolete file)
1.29 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
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•19 years ago
|
||
I'll try to verify this later today.
Comment 3•19 years ago
|
||
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•19 years ago
|
||
bug 292962 / bug 311269 landed on MOZILLA_1_8_BRANCH on 20051018 1631pst causing the regression there aswell.
Keywords: regression
Reporter | ||
Updated•19 years ago
|
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Comment 5•19 years ago
|
||
windows sux. after a reboot i wasn't able to reproduce the problem on branch at all.
Comment 6•19 years ago
|
||
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
Reporter | ||
Comment 7•19 years ago
|
||
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•19 years ago
|
||
Please refer to posted patch(attachment 207342 [details]) to Bug 321671.
Depends on: 321671
Comment 9•19 years ago
|
||
posted patch is attachment 207341 [details] [diff] [review].
Comment 10•19 years ago
|
||
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•19 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•19 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.
Reporter | ||
Comment 13•19 years ago
|
||
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•19 years ago
|
||
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)
Reporter | ||
Comment 16•19 years ago
|
||
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?
Reporter | ||
Comment 18•19 years ago
|
||
I don't think there's a need for that with this patch...
Assignee | ||
Comment 19•19 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•18 years ago
|
Flags: blocking1.9a1?
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.
Description
•