Closed Bug 671993 Opened 10 years ago Closed 6 years ago

Failure to save forms data in session results in data loss

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
fennec 38+ ---

People

(Reporter: CoJaBo-Bugzilla, Assigned: mfinkle)

References

Details

Attachments

(5 files, 4 obsolete files)

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.
This bug was opened as a followup to bug 630398.
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.
Severity: critical → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: All → ARM
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.
Product: Fennec → Fennec Native
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!!
tracking-fennec: --- → ?
Assignee: nobody → mark.finkle
tracking-fennec: ? → 38+
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.
Attachment #8548915 - Flags: review?(bnicholson)
Attached patch save-moar-session WIP (obsolete) — Splinter Review
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.
Attachment #8548915 - Flags: review?(bnicholson) → review+
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.
Attachment #8548918 - Attachment is obsolete: true
Attachment #8550839 - Flags: review?(bnicholson)
Attached patch session-telemetry v0.1 (obsolete) — Splinter Review
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.
Attachment #8550840 - Flags: review?(bnicholson)
Attached patch session-flush-not-sync v0.1 (obsolete) — Splinter Review
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.
Attachment #8550841 - Flags: review?(bnicholson)
Working on tests now...
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
Attachment #8550840 - Attachment is obsolete: true
Attachment #8550840 - Flags: review?(bnicholson)
Attachment #8551449 - Flags: review?(bnicholson)
Attached patch session-flush-not-sync v0.2 (obsolete) — Splinter Review
Rebased
Attachment #8550841 - Attachment is obsolete: true
Attachment #8550841 - Flags: review?(bnicholson)
Attachment #8551451 - Flags: review?(bnicholson)
Attachment #8550839 - Flags: review?(bnicholson) → review+
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.
Attachment #8551449 - Flags: review?(bnicholson) → review+
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
(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.
Uses old sync file writing code block, but uses a fake Promise to allow the calling code to not duplicate code.
Attachment #8551451 - Attachment is obsolete: true
Attachment #8551451 - Flags: review?(bnicholson)
Attachment #8552517 - Flags: review?(bnicholson)
Comment on attachment 8552517 [details] [diff] [review]
session-promises v0.1

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

Nice, I like it.
Attachment #8552517 - Flags: review?(bnicholson) → review+
(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.
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
Attachment #8553115 - Flags: review?(bnicholson)
(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 on attachment 8553115 [details] [diff] [review]
session-tests v0.1

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

yay
Attachment #8553115 - Flags: review?(bnicholson) → review+
Duplicate of this bug: 1017927
You need to log in before you can comment on or make changes to this bug.