671993 - Failure to save forms data in session results in data loss

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[CoJaBo]

Go to any site that has a form, enter some text, then switch to another memory-intensive app. This should cause Firefox to be unloaded.
When Fennec is switched back to and starts back up, the form data is not maintained.
(This also seems to happen when switching away from the form tab to another for a while, causing it to reload, but I haven't tried to replicate that.)

This is expected to happen fairly frequently on a mobile device (e.g., receive a phone call while composing a long email, and have to start over), so is a rather serious usability concern.

This probably depends on Bug 656062 being fixed as well, as maintaining session data across reloads isn't properly implemented in the first place.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

This bug was opened as a followup to bug 630398.

Comment 2

[Carla Nadastean]

I was able to reproduce bug with:
Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110908 Firefox/9.0a1 Fennec/9.0a1
Device: HTC Desire Z
OS: Android 2.3

Steps to reproduce:
1. Open Fennec app.
2. Browse to a webpage that have a username field . (e.g www.facebook.com)
3. Fill in the username field with some text.
4. Tap on System home button.
5. Open and use some other applications. (e.g Twitter)
6. Switch back to Fennec app.
7. Verify text is still displayed in username field.

Expected Result:
Text is still displayed in username field.

Actual Result:
Field is empty.

Comment 3

[CoJaBo]

This still affects the latest Fennec Native nightly; receiving a phonecall while commenting on, e.g., a bug, will result in the loss of that comment.

Comment 4

[Manu Jain]

I would like to patch up this bug. Can someone assist me that in which files we need to make changes i.e "http://mxr.mozilla.org/mozilla-central/". Thanks!!

Comment 5

[Martijn Wargers (dead)]

These are the Android files:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.idl?force=1
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js?force=1

On desktop, we have something called formData that is stored:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#343
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/sessionstore/FormData.jsm

So I guess we need something similar to that.
But on mobile, it probably needs to be less complete, because of memory constraints or something.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: no-session-for-dynamic-frames v0.1 (fixup)

I found two issues that should be addressed regardless:
1. Desktop does not save history entries for dynamic frames
2. We had a bug where the history entry "children" where not being saved because the code was in the wrong scope.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: save-moar-session WIP

This patch adds form data to the session using the same FormData.jsm as Desktop uses. We also add the data to the session JSON file in the same way as Desktop.

I need more testing to make sure frames are working OK. So far, it seems like "restore after quit/crash" and "undo close tab" are restoring the form data correctly.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: save-moar-session v0.1

I think this is ready for review. A few notes though:
1. Places where we called restoreHistory now call restoreTab, since we restore more than history now.
2. We use "input", "change" and "DOMAutoComplete" events as triggers for saving form data just like Desktop.
3. We can't restore form data as early as we restore history. We have to wait for the document to load so we can update the values. We use the "load" event just like Desktop.
4. We use a pref to determine if we should save form data from HTTPS pages. It's defaulted to the same as Desktop (save form data on all pages)

I also have some {} formatting in there.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: session-telemetry v0.1

Simple patch adds telemetry for three histograms:
* SERIALIZE_DATA_MS (how fast do we JSON stringify the state)
* FX_SESSION_RESTORE_WRITE_FILE_MS (time to write the file)
* FX_SESSION_RESTORE_FILE_SIZE_BYTES (save of the encoded data)

We remove the code that sends the state string out with the "sessionstore-state-write" notification. Desktop removed that in bug 944557.

We add the "sessionstore-state-write-complete" notification that was missing in the Sync code block.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: session-flush-not-sync v0.1

In bug 965137 you added a Sync file writing code path, along with changes to force a state save on going to the background. You also used DOMTitleChanged to trigger a new load session save since it's earlier than pageshow.

I found that OS.File.writeAtomic has a way to flush to disk right away. I want to try that, which along with the other fixes you made, should still be flushing the state to disk ASAP and not losing any state.

This cleans up the code in _writeState quite a bit too.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Working on tests now...

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: session-telemetry v0.2

I had the wrong name for TelemetryStopwatch and I didn't have the full name for "FX_SESSION_RESTORE_SERIALIZE_DATA_MS".

I can now confirm that data is being collected via about:telemetry

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: session-flush-not-sync v0.2

Rebased

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 8551449 [details] [diff] [review]
session-telemetry v0.2

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

::: mobile/android/components/SessionStore.js
@@ +629,2 @@
>  
> +    TelemetryStopwatch.start("FX_SESSION_RESTORE_WRITE_FILE_MS");

Can we have different telemetry events for sync/async writes? Only relevant if we keep sync, of course; see next comment.

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 8551451 [details] [diff] [review]
session-flush-not-sync v0.2

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

(In reply to Mark Finkle (:mfinkle) from comment #10)
> I found that OS.File.writeAtomic has a way to flush to disk right away. I
> want to try that, which along with the other fixes you made, should still be
> flushing the state to disk ASAP and not losing any state.

Reading the docs at [1], it looks like flush is a lower-level OS operation that provides added safety in the case of an OS crash, power failure, etc., but the write still happens asynchronously. In other words, the write itself is more robust, but there's still a delay before that write happens. The app's process may be killed immediately after onPause() finishes, so any pending async operations still might not be executed.

Given the small window, it's difficult to reproduce cases where sync vs. async writes matter, so it's hard to say how big of a deal this is in practice -- but I'd still lean toward sync writes just to err on the safe side. Just to be sure, I asked this question in #android-dev, and they confirmed that writes in onPause() must be sync to avoid data loss.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread#OS.File.writeAtomic%28%29

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brian Nicholson (:bnicholson) from comment #14)
 
> > +    TelemetryStopwatch.start("FX_SESSION_RESTORE_WRITE_FILE_MS");
> 
> Can we have different telemetry events for sync/async writes? Only relevant
> if we keep sync, of course; see next comment.

I'd rather not add something that's not also used in Desktop.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: session-promises v0.1

Uses old sync file writing code block, but uses a fake Promise to allow the calling code to not duplicate code.

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 8552517 [details] [diff] [review]
session-promises v0.1

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

Nice, I like it.

Comment 19

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #16)
> (In reply to Brian Nicholson (:bnicholson) from comment #14)
>  
> > > +    TelemetryStopwatch.start("FX_SESSION_RESTORE_WRITE_FILE_MS");
> > 
> > Can we have different telemetry events for sync/async writes? Only relevant
> > if we keep sync, of course; see next comment.
> 
> I'd rather not add something that's not also used in Desktop.

OK, though if we see unusually long writes being reported, one of the first things we'll probably want to know is which of the two types the write falls under. I guess we can always add this reporting later if it turns out to be an issue.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: session-tests v0.1

Last one for this bug. Tests!

And they seem to be passing locally.

I borrowed from Desktop here, except where Desktop has an elaborate set of messaging and a library of helper methods, I only added what I needed.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69169aba3c90

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #20)

> Try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=69169aba3c90

Android 2.3 test was failing, likely due to a timeout. I changed the test to use a | promiseBrowserLoad | instead of a | sleep | and it's working fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e68e5d0be9

Green!

Comment 22

[Brian Nicholson (:bnicholson)]

Comment on attachment 8553115 [details] [diff] [review]
session-tests v0.1

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

yay

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/b0dc75079685
remote:   https://hg.mozilla.org/integration/fx-team/rev/aca7a35ca960
remote:   https://hg.mozilla.org/integration/fx-team/rev/23300c6e21ea
remote:   https://hg.mozilla.org/integration/fx-team/rev/ae6a735c35a5
remote:   https://hg.mozilla.org/integration/fx-team/rev/651b6120a381

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0dc75079685
https://hg.mozilla.org/mozilla-central/rev/aca7a35ca960
https://hg.mozilla.org/mozilla-central/rev/23300c6e21ea
https://hg.mozilla.org/mozilla-central/rev/ae6a735c35a5
https://hg.mozilla.org/mozilla-central/rev/651b6120a381