regression: form content is lost when pressing back

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
DOM
P1
blocker
VERIFIED FIXED
13 years ago
7 years ago

People

(Reporter: José Jeria, Assigned: Brian Ryner (not reading))

Tracking

({dataloss, regression})

Trunk
mozilla1.8beta2
dataloss, regression
Points:
---
Bug Flags:
blocking1.8b2 +
blocking-aviary1.5 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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
(Reporter)

Updated

13 years ago
Severity: normal → critical
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. ***
(Reporter)

Updated

13 years ago
Flags: blocking1.8b2?
(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
Blocks: 274784
Flags: blocking-aviary1.1?
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
Flags: blocking1.8b2?
Flags: blocking1.8b2+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050506] (nightly) (W98SE)

(MAS too, confirming)
Keywords: dataloss
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

13 years ago
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.

Comment 15

13 years ago
set severity=blocker ?
Severity: blocker → normal
Component: DOM → History: Session
Priority: P1 → P2

Updated

13 years ago
Severity: normal → blocker
Component: History: Session → DOM
Priority: P2 → P1
(Assignee)

Comment 16

13 years ago
Created attachment 183080 [details] [diff] [review]
patch

Save state when we're detached from the window.
Attachment #183080 - Flags: superreview?(bzbarsky)
Attachment #183080 - Flags: review?(bzbarsky)
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...
Attachment #183080 - Flags: superreview?(bzbarsky)
Attachment #183080 - Flags: superreview+
Attachment #183080 - Flags: review?(bzbarsky)
Attachment #183080 - Flags: review+
(Assignee)

Comment 19

13 years ago
(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?
(Assignee)

Comment 20

13 years ago
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?
> 

(Assignee)

Comment 21

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

Comment 23

13 years ago
Created attachment 183104 [details] [diff] [review]
like this
Attachment #183080 - Attachment is obsolete: true
Attachment #183104 - Flags: superreview?(bzbarsky)
Attachment #183104 - Flags: review?(bzbarsky)
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.
Attachment #183104 - Flags: superreview?(bzbarsky)
Attachment #183104 - Flags: superreview+
Attachment #183104 - Flags: review?(bzbarsky)
Attachment #183104 - Flags: review+
Attachment #183104 - Flags: approval1.8b2?
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+
(Assignee)

Comment 26

13 years ago
checked in.
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).

Comment 29

13 years ago
(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

Comment 32

13 years ago
Wrong bug number I think... (In reply to comment #31)
> I filed bug 274784 on the Tp issue.

Comment 33

13 years ago
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.

Comment 36

13 years ago
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.

Comment 38

13 years ago
(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.