Build id: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050505 Firefox/1.0+ This is a fresh regression. It works in build 20050504, but not in 20050505. I have NOT enabled the new "fastback/bfcache" feature Steps to reproduce: 1. go to https://bugzilla.mozilla.org/query.cgi 2. search for something (type in anyting in the summary field) 3. search results are shown, now press back, notice that summary field is empty
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050505 Firefox/1.0+ confirming, this happens with bfcache both ON and OFF
*** Bug 293024 has been marked as a duplicate of this bug. ***
is this a duplicate of bug #133946? https://bugzilla.mozilla.org/show_bug.cgi?id=133946
(In reply to comment #3) > is this a duplicate of bug #133946? > https://bugzilla.mozilla.org/show_bug.cgi?id=133946 No, this regressed on the 5th
Yeah, form state restoration on back is just completely busted. Try it with any bug page. Chances are, when we go to store the data, there is no script global on the document anymore (since DOM tree teardown is later now) and we just bail out...
Assignee: nobody → bryner
OS: Windows XP → All
Hardware: PC → All
Other things that could have caused this are the document's container being null (see nsGenericHTMLElement::GetLayoutHistoryAndKey()) or the docshell having the wrong mOSHE (see nsDocShell::GetLayoutHistoryState). This last is a problem for fastback, since evicted content viewers will lose their form data, or worse yet store it in the wrong SHEntry. I can file a followup bug on that depending on what the fix here ends up being.
Need a fix ASAP. This is a blocker. /be
Severity: critical → blocker
Component: History: Session → DOM
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050506] (nightly) (W98SE) (MAS too, confirming)
It appears that the form control data is getting saved into the LayoutHistoryState for the session history entry of the new document, instead of the old document.
I think the root cause of the problem is that the UnbindFromTree calls now happen inside of DocumentViewer::Destroy(), instead of in Close(). The latter is called before the docshell's history entry is swapped out; the former is called well afterwards. The easiest way to solve this is probably to destroy the document in Close() if we're not going to be saving the presentation....
Actually that solution is incomplete, because then we won't have the saved history state if the content viewer is later evicted from session history. One thing we could do is create a stronger association between a document and its LayoutHistoryState. That is, rather than have the document get whatever history entry is current on the docshell, it would actually have a pointer to the LHS to save state into. Another approach would be to separate state-saving from teardown, so that we could always save state from Close(). That might be safer as far as preserving existing behavior. Boris, Johnny, any thoughts?
Does Close() run before or after unload handlers? At the moment, we persist the state of nodes removed from the document in unload handlers (or at any time, in fact). I'm not sure whether that's a completely desirable behavior, frankly, but it would make sense to me to save state before we "navigate away from the document" (so in SetScriptGlobalObject(nsnull) or something equivalent)...
*** Bug 293402 has been marked as a duplicate of this bug. ***
Probably the best way to preserve current behavior short-term would be to grab a pointer to the nsILayoutHistoryState in the document when SetScriptGlobalObject(nsnull) happens, and forget it in nsDocument::Destroy() after unhooking our kids. Probably want to forget it when we get a new script global too. Then we can take our time sorting out exactly when we want to save element state, how it should interact with unload, etc.
set severity=blocker ?
Severity: blocker → normal
Component: DOM → History: Session
Priority: P1 → P2
Severity: normal → blocker
Component: History: Session → DOM
Priority: P2 → P1
Created attachment 183080 [details] [diff] [review] patch Save state when we're detached from the window.
Again, this changes our current state-saving behavior. We will no longer persist the state of any node removed from the document before SetScriptGlobalObject. I'd really rather we didn't make a substantive change like that at this point, unless we're very very sure this won't break state restoration on existing pages.
Comment on attachment 183080 [details] [diff] [review] patch r+sr=bzbarsky just as far as fixing the immediate regression, but I'd really like to see a fix the restores the original behavior...
(In reply to comment #17) > Again, this changes our current state-saving behavior. We will no longer > persist the state of any node removed from the document before > SetScriptGlobalObject. I'd really rather we didn't make a substantive change > like that at this point, unless we're very very sure this won't break state > restoration on existing pages. I'm not sure I follow -- this used to happen via DocumentViewer::Close() calling SetScriptGlobalObject(null), and with this patch we'll be back to doing that. What is different?
Oh, because nodes would independently save their state on removal from the document. Never mind. (In reply to comment #19) > I'm not sure I follow -- this used to happen via DocumentViewer::Close() calling > SetScriptGlobalObject(null), and with this patch we'll be back to doing that. > What is different? >
We could continue to call SaveState from UnbindFromTree, if the document has a ScriptGlobalObject pointer (since in that case, the docshell will have the correct history entry available). That seems like it would address this problem.
Yes, that would work quite nicely. Let's do that.
Created attachment 183104 [details] [diff] [review] like this
Comment on attachment 183104 [details] [diff] [review] like this Most righteous. Could this please be approved for 1.8b2? Makes form state restoration happy again and all.
Comment on attachment 183104 [details] [diff] [review] like this a=shaver for sweet regression fix. Thanks to both of you.
Attachment #183104 - Flags: approval1.8b2? → approval1.8b2+
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Looks like this hurt Tp a bit.
Probably because we're now doing two walks over the full DOM on page unload instead of just one walk... I can't think of a good way to fix that other than going back to just storing state in UnbindFromTree and making sure we get the right layout state (probably by storing it in the document).
(In reply to comment #26) > checked in. WMF (Linux/SM), Thanks!
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050510] (nightly) (W98SE) V.Fixed.
Status: RESOLVED → VERIFIED
I filed bug 274784 on the Tp issue.
yes, the bug he filed on the tp issue was bug 293709
I filed a regression that is bug 293793.
Unfortunately, this didn't fully fix things... Bug 293588 has more analysis.
By reloading, form content disappears/reappears. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050514 Firefox/1.0+ How to reproduce: 1. Write something in Additional Comments in this page 2. Reload, and reload again Is this behavior caused by this bug or is it new?
It's caused by the fix for bug 293588, more or less (and by bug 274784 in the end). I've filed bug 294258 on the issue.
(In reply to comment #37) > It's caused by the fix for bug 293588, more or less (and by bug 274784 in the > end). I've filed bug 294258 on the issue. So, is it really even fixed then? Should this just be a duplicate of 274784 then?
> So, is it really even fixed then? Yes. > Should this just be a duplicate of 274784 then? No.
You need to log in before you can comment on or make changes to this bug.