Closed Bug 1261225 Opened 4 years ago Closed 4 years ago

Improve session store form data handling

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(3 files)

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.
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/
Attachment #8737272 - Flags: review?(margaret.leibovic)
Attachment #8737273 - Flags: review?(margaret.leibovic)
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 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?
Attachment #8737272 - Flags: review?(margaret.leibovic) → review+
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
Attachment #8737273 - Flags: review?(margaret.leibovic) → review+
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 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 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/
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 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 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 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/
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 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 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?
Attachment #8737871 - Flags: review?(margaret.leibovic) → review+
Blocks: 1262565
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 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 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 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/
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 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/
You need to log in before you can comment on or make changes to this bug.