1261225 - Improve session store form data handling

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

Fallout from bug 852267:
- The variable used for form data restoring could use a more descriptive name, plus the code flow with the copy of the session data can be a bit confusing.
- Saved form data might still get lost if a tab/Firefox is closed/killed at an inopportune moment.
- Forward/back key navigation can lead to data loss:
1. Enter some text in some input box (e.g. the Bugzilla comment box).
2. Navigate to some other page.
3. Go back (text you entered is still present).
4. Close the tab (or have it zombified or kill Firefox).
5. Restore the tab.

Expected: Form data is restored.
Actual: Form is empty.

This is because after a DOMTitleChanged event, we throw away all previous session data and recreate only the history part of it immediately. Form data is only saved when the next input event arrives, so if the user doesn't interact with the form after going back/forward, the form data - which is restored internally by Gecko without involving the session store as long as the tab is kept in memory - won't be captured by the session store.

Comment 1

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1261225 - Part 1 - Refactor text data restoring. r=margaret

Previously, when restoring a tab, we made a copy of the whole session data from which we would then extract and restore the form data - if present - after page loading had completed.

With the changes from bug 852267, the original session data - containing any eventual form data - is now always reattached to the restored tab object, therefore we no longer need to make a copy of it. Instead, OnTabLoad() has been adapted to preserve the formdata if it is needed for restoring later on.

Review commit: https://reviewboard.mozilla.org/r/43863/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43863/

Comment 2

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1261225 - Part 2 - Always capture form data on page navigation. r=margaret

As long as a tab is kept in memory, Gecko handles restoring of the form data during forward/back key navigation internally without involving the session store. After going back/forward, OnTabLoad() is triggered, which throws away the previous session store data for that tab and recreates only the session history part of it. Form data is only captured in OnTabInput(), which means that if Firefox is killed or the tab closed/zombified before the user interacts with the form (triggering input events), form contents won't be captured by the session store and will therefore get lost.

Review commit: https://reviewboard.mozilla.org/r/43865/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43865/

Comment 3

[:Margaret Leibovic]

Comment on attachment 8737272 [details]
MozReview Request: Bug 1261225 - Part 1 - Refactor text data restoring. r=margaret

https://reviewboard.mozilla.org/r/43863/#review40517

I see in bug 1044556 you started writing some session restore tests. Could you write a test for restoring this form data?

Comment 4

[:Margaret Leibovic]

Comment on attachment 8737273 [details]
MozReview Request: Bug 1261225 - Part 2 - Always capture form data on page navigation. r=margaret

https://reviewboard.mozilla.org/r/43865/#review40519

Comment 5

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/43863/#review40517

It looks like we [already have a test for this](https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_session_form_data.html), but I'll try extending it to cover the back/forward navigation case from part 2.

Comment 6

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1261225 - Part 3 - Extend session store form data test to cover capturing form data after back/forward navigation. r=margaret

Review commit: https://reviewboard.mozilla.org/r/44153/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44153/

Comment 7

[Jan Henning [:JanH]]

Comment on attachment 8737272 [details]
MozReview Request: Bug 1261225 - Part 1 - Refactor text data restoring. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43863/diff/1-2/

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8737273 [details]
MozReview Request: Bug 1261225 - Part 2 - Always capture form data on page navigation. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43865/diff/1-2/

Comment 9

[Jan Henning [:JanH]]

I'll probably hold off on landing Part 3 until after bug 1044556 lands, which means that promiseTabEvent() will be available from head.js. As I'd like to uplift bug 1044556, I'd prefer keeping that change there.

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8737272 [details]
MozReview Request: Bug 1261225 - Part 1 - Refactor text data restoring. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43863/diff/2-3/

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8737273 [details]
MozReview Request: Bug 1261225 - Part 2 - Always capture form data on page navigation. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43865/diff/2-3/

Comment 12

[Jan Henning [:JanH]]

Comment on attachment 8737871 [details]
MozReview Request: Bug 1261225 - Part 3 - Extend session store form data test to cover capturing form data after back/forward navigation. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44153/diff/1-2/

Comment 13

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/43863/#review40517

And a good thing it was, too, because it turned out in my first patch I broke things [around here](https://reviewboard-hg.mozilla.org/gecko/rev/0666d6febc55#l1.19), because I was unconditionally accessing `aBrowser.__SS_data.formdata`, which would throw if `__SS_data` was null, which it would be when opening a new tab.

Comment 14

[Jan Henning [:JanH]]

Comment on attachment 8737871 [details]
MozReview Request: Bug 1261225 - Part 3 - Extend session store form data test to cover capturing form data after back/forward navigation. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44153/diff/2-3/

Comment 15

[:Margaret Leibovic]

Comment on attachment 8737871 [details]
MozReview Request: Bug 1261225 - Part 3 - Extend session store form data test to cover capturing form data after back/forward navigation. r=margaret

https://reviewboard.mozilla.org/r/44153/#review41291

Thanks for the test! This looks good to me, although it could be worth filing a follow-up to investigate a way to avoid the sleep statements, especially if we plan to add more session restore tests (we should!).

::: mobile/android/tests/browser/chrome/test_session_form_data.html:256
(Diff revision 3)
> +      // Note: Gecko currently doesn't restore the inner form data after navigation,
> +      // so we can't check for the inner form value.
> +      todo_is(getInputValue(browser, {id: "txt", frame: 0}), INNER_VALUE, "inner value present after navigation");
> +
> +      // Allow the session store a bit of time to actually capture the form data.
> +      yield sleep(CLOSE_TAB_WAIT);

This makes me a bit nervous... could we add a notificaiton somewhere in the session restore code to help make tests more deterministic?

Comment 16

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/44153/#review41291

> This makes me a bit nervous... could we add a notificaiton somewhere in the session restore code to help make tests more deterministic?

I've filed a [follow-up bug](https://bugzilla.mozilla.org/show_bug.cgi?id=1262565) for this, which I'll do next.

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8737272 [details]
MozReview Request: Bug 1261225 - Part 1 - Refactor text data restoring. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43863/diff/3-4/

Comment 18

[Jan Henning [:JanH]]

Comment on attachment 8737273 [details]
MozReview Request: Bug 1261225 - Part 2 - Always capture form data on page navigation. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43865/diff/3-4/

Comment 19

[Jan Henning [:JanH]]

Comment on attachment 8737871 [details]
MozReview Request: Bug 1261225 - Part 3 - Extend session store form data test to cover capturing form data after back/forward navigation. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44153/diff/3-4/

Comment 20

[Jan Henning [:JanH]]

Strangely enough after rebasing to the current central and testing again, the inner value tests started passing again, so I've re-enabled them.

Comment 21

[Jan Henning [:JanH]]

Comment on attachment 8737871 [details]
MozReview Request: Bug 1261225 - Part 3 - Extend session store form data test to cover capturing form data after back/forward navigation. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44153/diff/4-5/

Comment 22

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38603cb40a76

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4972c9a34bca
https://hg.mozilla.org/integration/fx-team/rev/9c39e23f6580
https://hg.mozilla.org/integration/fx-team/rev/a6c06f1c8fb6

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4972c9a34bca
https://hg.mozilla.org/mozilla-central/rev/9c39e23f6580
https://hg.mozilla.org/mozilla-central/rev/a6c06f1c8fb6