293135 - regression: form content is lost when pressing back

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[José Jeria]

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

Comment 1

[Peter van der Woude [:Peter6]]

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

Comment 2

[Peter van der Woude [:Peter6]]

*** Bug 293024 has been marked as a duplicate of this bug. ***

Comment 3

[Chris]

is this a duplicate of bug #133946?
https://bugzilla.mozilla.org/show_bug.cgi?id=133946

Comment 4

[Peter van der Woude [:Peter6]]

(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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Brendan Eich [:brendan]]

Need a fix ASAP.  This is a blocker.

/be

Comment 8

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050506] (nightly) (W98SE)

(MAS too, confirming)

Comment 9

[Brian Ryner (not reading)]

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.

Comment 10

[Brian Ryner (not reading)]

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....

Comment 11

[Brian Ryner (not reading)]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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)...

Comment 13

[:Gavin Sharp [email: [e-mail]]]

*** Bug 293402 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

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

[Wayne Mery (:wsmwk)]

set severity=blocker ?

Comment 16

[Brian Ryner (not reading)]

Attachment: patch

Save state when we're detached from the window.

Comment 17

[Boris Zbarsky [: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 18

[Boris Zbarsky [:bzbarsky]]

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...

Comment 19

[Brian Ryner (not reading)]

(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?

Comment 20

[Brian Ryner (not reading)]

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?
> 

Comment 21

[Brian Ryner (not reading)]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yes, that would work quite nicely.  Let's do that.

Comment 23

[Brian Ryner (not reading)]

Attachment: like this

Comment 24

[Boris Zbarsky [: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.

Comment 25

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 183104 [details] [diff] [review]
like this

a=shaver for sweet regression fix.  Thanks to both of you.

Comment 26

[Brian Ryner (not reading)]

checked in.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like this hurt Tp a bit.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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

[Volker Moell]

(In reply to comment #26)
> checked in.
WMF (Linux/SM), Thanks!

Comment 30

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050510] (nightly) (W98SE)

V.Fixed.

Comment 31

[Boris Zbarsky [:bzbarsky]]

I filed bug 274784 on the Tp issue.

Comment 32

[franCk]

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

Comment 33

[Michael Lefevre]

yes, the bug he filed on the tp issue was bug 293709

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I filed a regression that is bug 293793.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Unfortunately, this didn't fully fix things... Bug 293588 has more analysis.

Comment 36

[Ryuichi KUBUKI]

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?

Comment 37

[Boris Zbarsky [:bzbarsky]]

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

[Worcester12345]

(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?

Comment 39

[Boris Zbarsky [:bzbarsky]]

> So, is it really even fixed then?

Yes.

> Should this just be a duplicate of 274784 then?

No.