873835 - Input text removed after restart

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dan Wolff]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20130518 Firefox/23.0 (Nightly/Aurora)
Build ID: 20130518004019

Steps to reproduce:

0. Set the browser to keep tabs from the previous session ("Show my windows and tabs from last time")
1. Go to the URL 'data:text/html,<textarea>'
2. Write something in the text area
3. Restart the browser


Actual results:

The text in the text area is gone.


Expected results:

The text entered should have been kept.

This is a regression from Firefox 22.

Comment 1

[Alice0775 White]

I can reproduce the problem with he STR in comment#0, but it happens intermittently.

Following STR reproduces a problem with 100% of probability.

Steps to reproduce:
0. Set the browser to keep tabs from the previous session ("Show my windows and tabs from last time")
1. Go to the URL 'data:text/html,<textarea>'
2. Write something in the text area
2-1. Wait about 15 seconds
3. Restart the browser

Actual results:
The text in the text area is gone.

Alternative Steps to Reproduce:
1. Open 'data:text/html,<textarea>' in a new tab [Tab A]
2. Write something in the text area
3. Open about:newTab (Ctrl+T) [Tab B]
5. Wait about 15 seconds
6. Close [Tab A] by clicking close button in the tab
7. Perform "Undo close tab" 

Actual results:
[Tab A] is restored.
However, the text in the text area is gone.

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/b109e2dbf03b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130505 Firefox/23.0 ID:20130505171547
Bad:
http://hg.mozilla.org/mozilla-central/rev/dae38fc0aeb4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130506 Firefox/23.0 ID:20130506043045
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b109e2dbf03b&tochange=dae38fc0aeb4

Regression window(fx)
Good:
http://hg.mozilla.org/integration/fx-team/rev/a43afeb91832
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130501 Firefox/23.0 ID:20130501140431
Bad:
http://hg.mozilla.org/integration/fx-team/rev/4a496e6b99af
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130501 Firefox/23.0 ID:20130502031430
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a43afeb91832&tochange=4a496e6b99af

Regressed by: 
4a496e6b99af	Tim Taubert — Bug 867097 - Remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property; r=yoric

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: re-implement form data cache now that __SS_data is gone

The problem here is that _SS_formDataSaved was just a flag that said whether we should update form data or just read it from __SS_data. Now that the __SS_data cache is gone we need another place to store this data and I seized the chance to implement a proper cache.

Good thing is that with __SS_formDataSaved another ugly property will be gone.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: re-implement form data cache now that __SS_data is gone

Small cleanup.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 752129 [details] [diff] [review]
re-implement form data cache now that __SS_data is gone

Review of attachment 752129 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Is browser_input.js sufficient to check that we have successfully replaced __SS_data?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1337,5 @@
>     * @param aBrowser
>     *        Browser reference
>     */
>    onTabInput: function ssi_onTabInput(aWindow, aBrowser) {
> +    FormDataCache.remove(aBrowser);

Is there any reason you removed the comment?

@@ +2166,5 @@
>     * @param aFullData
>     *        always return privacy sensitive data (use with care)
>     */
>    _updateTextAndScrollDataForTab:
> +    function ssi_updateTextAndScrollDataForTab(aBrowser, aTabData, aFullData) {

Note to self: if there's a bug when we merge, look here :)

@@ +2236,5 @@
>            formData.id["sessionData"] = JSON.parse(formData.id["sessionData"]);
>          }
>  
>          if (Object.keys(formData.id).length ||
>              Object.keys(formData.xpath).length) {

Side-note: |Object.keys(...).length|? That looks expensive.

@@ +2243,5 @@
> +          formData = null;
> +        }
> +
> +        // Store form data in cache.
> +        FormDataCache.set(aBrowser, aContent, formData);

I understand how that can be useful, but not how previous versions managed to get away without doing this. Or is this part of the __SS_data you removed?

@@ +2248,5 @@
> +      } else {
> +        // Copy from form data cache.
> +        let cached = FormDataCache.get(aBrowser, aContent);
> +        if (cached) {
> +          aData.formdata = cached;

Are we sure that aData.formdata is not modified? Perhaps we should freeze it somehow?

@@ +4672,5 @@
>  
> +// A map storing cached form data belonging to browsers. Each browser itself
> +// has a WeakMap assigned that holds the form data of its main and subframes.
> +let FormDataCache = {
> +  _cache: new WeakMap(),

You should document more clearly that it's a WeakMap<browser, WeakMap<frame, data>>

::: browser/components/sessionstore/test/browser_input.js
@@ +54,5 @@
>  
> +  // Check that the form data cache works properly.
> +  let {entries: [{formdata}]} = JSON.parse(ss.getTabState(tab));
> +  is(formdata.id.chk, true, "chk's cached value is correct");
> +  is(formdata.id.txt, "m", "txt's cached value is correct");

What are we checking here, exactly?

@@ +75,5 @@
> +  is(formdata.id.chk, true, "iframe chk's value is correct");
> +  is(formdata.id.txt, "m", "iframe txt's value is correct");
> +
> +  // Check that the form data cache works properly for subframes.
> +  let {entries: [{children: [{formdata}]}]} = JSON.parse(ss.getTabState(tab));

I don't see any difference with the previous lines.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 752129 [details] [diff] [review]
re-implement form data cache now that __SS_data is gone

Review of attachment 752129 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Is browser_input.js sufficient to check that we have successfully replaced __SS_data?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1337,5 @@
>     * @param aBrowser
>     *        Browser reference
>     */
>    onTabInput: function ssi_onTabInput(aWindow, aBrowser) {
> +    FormDataCache.remove(aBrowser);

Is there any reason you removed the comment?

@@ +2166,5 @@
>     * @param aFullData
>     *        always return privacy sensitive data (use with care)
>     */
>    _updateTextAndScrollDataForTab:
> +    function ssi_updateTextAndScrollDataForTab(aBrowser, aTabData, aFullData) {

Note to self: if there's a bug when we merge, look here :)

@@ +2236,5 @@
>            formData.id["sessionData"] = JSON.parse(formData.id["sessionData"]);
>          }
>  
>          if (Object.keys(formData.id).length ||
>              Object.keys(formData.xpath).length) {

Side-note: |Object.keys(...).length|? That looks expensive.

@@ +2243,5 @@
> +          formData = null;
> +        }
> +
> +        // Store form data in cache.
> +        FormDataCache.set(aBrowser, aContent, formData);

I understand how that can be useful, but not how previous versions managed to get away without doing this. Or is this part of the __SS_data you removed?

@@ +2248,5 @@
> +      } else {
> +        // Copy from form data cache.
> +        let cached = FormDataCache.get(aBrowser, aContent);
> +        if (cached) {
> +          aData.formdata = cached;

Are we sure that aData.formdata is not modified? Perhaps we should freeze it somehow?

@@ +4672,5 @@
>  
> +// A map storing cached form data belonging to browsers. Each browser itself
> +// has a WeakMap assigned that holds the form data of its main and subframes.
> +let FormDataCache = {
> +  _cache: new WeakMap(),

You should document more clearly that it's a WeakMap<browser, WeakMap<frame, data>>

::: browser/components/sessionstore/test/browser_input.js
@@ +54,5 @@
>  
> +  // Check that the form data cache works properly.
> +  let {entries: [{formdata}]} = JSON.parse(ss.getTabState(tab));
> +  is(formdata.id.chk, true, "chk's cached value is correct");
> +  is(formdata.id.txt, "m", "txt's cached value is correct");

What are we checking here, exactly?

@@ +75,5 @@
> +  is(formdata.id.chk, true, "iframe chk's value is correct");
> +  is(formdata.id.txt, "m", "iframe txt's value is correct");
> +
> +  // Check that the form data cache works properly for subframes.
> +  let {entries: [{children: [{formdata}]}]} = JSON.parse(ss.getTabState(tab));

I don't see any difference with the previous lines.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> Is browser_input.js sufficient to check that we have successfully replaced
> __SS_data?

I started writing a test only to test this behavior and it ended up looking exactly like browser_input.js just without the double-call of getTabState(). I do think it is sufficient. I couldn't find any similar places while skimming the code.

> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +1337,5 @@
> >     * @param aBrowser
> >     *        Browser reference
> >     */
> >    onTabInput: function ssi_onTabInput(aWindow, aBrowser) {
> > +    FormDataCache.remove(aBrowser);
> 
> Is there any reason you removed the comment?

No, I just thought the line doesn't really need a comment anymore as it's a little more expressive now.

> >    _updateTextAndScrollDataForTab:
> > +    function ssi_updateTextAndScrollDataForTab(aBrowser, aTabData, aFullData) {
> 
> Note to self: if there's a bug when we merge, look here :)

Yeah...... :)

> >          if (Object.keys(formData.id).length ||
> >              Object.keys(formData.xpath).length) {
> 
> Side-note: |Object.keys(...).length|? That looks expensive.

I have no idea. Don't think there's a better of doing this, though.

> > +        // Store form data in cache.
> > +        FormDataCache.set(aBrowser, aContent, formData);
> 
> I understand how that can be useful, but not how previous versions managed
> to get away without doing this. Or is this part of the __SS_data you removed?

We previously had a flag telling us whether there's something cached (even if that meant that no form data has been found). Now we're looking up values in WeakMaps and so we need to store at least null so that we don't continously try to read form data from pages that don't have any.

> @@ +2248,5 @@
> > +      } else {
> > +        // Copy from form data cache.
> > +        let cached = FormDataCache.get(aBrowser, aContent);
> > +        if (cached) {
> > +          aData.formdata = cached;
> 
> Are we sure that aData.formdata is not modified? Perhaps we should freeze it
> somehow?

Nobody *should* but using Object.freeze() to guarantee that sounds good, will add.

> > +// A map storing cached form data belonging to browsers. Each browser itself
> > +// has a WeakMap assigned that holds the form data of its main and subframes.
> > +let FormDataCache = {
> > +  _cache: new WeakMap(),
> 
> You should document more clearly that it's a WeakMap<browser, WeakMap<frame,
> data>>

Good point.

> > +  // Check that the form data cache works properly.
> > +  let {entries: [{formdata}]} = JSON.parse(ss.getTabState(tab));
> > +  is(formdata.id.chk, true, "chk's cached value is correct");
> > +  is(formdata.id.txt, "m", "txt's cached value is correct");
> 
> What are we checking here, exactly?

We're checking that the second call to getTabState() in a row returns the correct tabState including the correct form data. This fails without the patch.

> > +  is(formdata.id.chk, true, "iframe chk's value is correct");
> > +  is(formdata.id.txt, "m", "iframe txt's value is correct");
> > +
> > +  // Check that the form data cache works properly for subframes.
> > +  let {entries: [{children: [{formdata}]}]} = JSON.parse(ss.getTabState(tab));
> 
> I don't see any difference with the previous lines.

Yeah, it's meant to be the same because getTabState() should read form data only once and then return the cached value if not modified in between.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/2d58f11fc294

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2d58f11fc294

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Bug 867097 landed in Firefox 23 - Firefox 22 is unaffected.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Backed out:

https://hg.mozilla.org/integration/fx-team/rev/14609c6d03cb

The cause, bug 867097, has been backed out as well.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Oh, sorry. Both patches are still on Fx 23 and need to be backed out separately.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Patches have been backed out from Aurora.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

This bug isn't valid anymore. It has been fixed by backing out a long time ago.