366302 - [SessionStore] only one text element/text area is saved

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

Steps to reproduce:
1. Load a page with several text elements (cf. URL)
2. Type something into several fields
3. Close the tab
4. Reopen the closed tab

Expected result:
All text elements are restored

Actual result:
Only the element which you've modified last is restored

Issue: For performance reasons, we store a reference to the text elements and verify that we haven't marked it for saving (cf. bug 344928). As it seems, hash keys are forced to strings though, making the key for all elements "[object XPCNativeWrapper [object HTMLInputElement]]"...

Comment 1

[Simon Bünzli]

Attachment: testcase

Comment 2

[Simon Bünzli]

Attachment: replace the one hash with two parallel vectors

Don't know how well this solution scales. Alternative solutions are thus welcome.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 250839 [details] [diff] [review]
replace the one hash with two parallel vectors

Ugh, this is an unfortunate bug. The fix looks ok, thanks. r=me.

Comment 4

[Simon Bünzli]

Could we get this checked in asap on Trunk for performance tests? This might be a late candidate for Firefox 2.0.0.2.

Comment 5

[Dietrich Ayala (:dietrich)]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.58; previous revision: 1.57
done

Comment 6

[Jay Patel [:jay]]

Not a real blocker, but marking so to keep it on the radar.  If this patch looks good on the Trunk after a few days, it'll be nice to take on the branches as well.  Please test thoroughly and ask for branch approvals if we are happy wiht the Trunk patch.

Comment 7

[Daniel Veditz [:dveditz]]

This changes the session data format slightly, doesn't it? How is conversion of
existing data handled (up, or even down given people who test with various
versions).  In particular people who have their start set to always restore
would have saved data in one format and then upgrade into the new code -- what
happens in that case?

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> This changes the session data format slightly, doesn't it? How is conversion of
> existing data handled (up, or even down given people who test with various
> versions).  In particular people who have their start set to always restore
> would have saved data in one format and then upgrade into the new code -- what
> happens in that case?
> 

The patch doesn't change the data format, only the in-memory structures used to reference the text fields.

Comment 9

[Simon Bünzli]

(In reply to comment #6)
> Please test thoroughly and ask for branch approvals if we are happy wiht the Trunk patch.

Well, are we happy with it? I haven't heard of any (performance) regressions nor witnessed any of them myself.

Comment 10

[tabmix.onemen]

some input filed don't saved:

in mozillazine forums the input filed for Knowledge Base Search

in http://tmp.garyr.net/forum the input filed for Quick Reply Subject

Comment 11

[Daniel Veditz [:dveditz]]

What version of Firefox is comment 10 talking about -- are you further describing the original bug or is that testing a trunk build that contains the patch?

Comment 12

[tabmix.onemen]

(In reply to comment #11)
> What version of Firefox is comment 10 talking about -- are you further
> describing the original bug or is that testing a trunk build that contains the
> patch?
> 

i see it on latest trunk with the patch

Comment 13

[Simon Bünzli]

(In reply to comment #12)
> i see it on latest trunk with the patch

Do you also see it on Trunk nightlies before the patch landed. In that respect, there really shouldn't be any regression.

We still don't save all form fields though (see e.g. bug 346337 comment #2).

Comment 14

[tabmix.onemen]

> (In reply to comment #12)

you are right, it is the same as before the patch.

but i examined sessionstore.js file and those form fields was saved.
look like the problem is in the restore part for some form fields.

Comment 15

[Simon Bünzli]

(In reply to comment #14)
> but i examined sessionstore.js file and those form fields was saved.

So please file a new bug with steps to reproduce. Thanks.

Comment 16

[Jay Patel [:jay]]

Comment on attachment 250839 [details] [diff] [review]
replace the one hash with two parallel vectors

Approved for 1.8 branch, a=jay for drivers.

Comment 17

[Simon Bünzli]

Dietrich/Gavin: Any chance for a check-in today before the freeze? Thanks.

Comment 18

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

Attachment: branch patch

On the branch, do we need to use wrappedTextArea everywhere?

Comment 19

[Simon Bünzli]

Attachment: branch patch for check-in

I knew I had forgotten about something... Sorry 'bout that.

Anyway, for the index we can't use the wrapped text element, since the reference to the wrapper is recreated every time the function is called whereas the reference we get is constant for the page.

Comment 20

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

mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.5.2.43

Comment 21

[Carsten Book [:Tomcat]]

Verified fixed for 1.8.1.2, on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.2pre) Gecko/2007020703 BonEcho/2.0.0.2pre - using the testcase from comment#1 and the steps to reproduce from comment #0. All text elements are restored from SessionStore

Comment 22

[Michael Kohler [:mkohler]]

Verified with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090618 Minefield/3.6a1pre ID:20090618031329

Comment 23

[u279076]

in-litmus-
There are several form tests that check this as a part of their Expected Results.  A specific test case is not needed for this bug IMO.