1301160 - A tab's parent ID doesn't get saved in session store

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

... meaning that if we're killed and the tabs are then subsequently restored via the session store, the feature of closing the current tab and returning to its parent when we reach the beginning of session history on pressing back stops working for those tabs.

What complicates a solution somewhat is that tab IDs aren't maintained across restarts, so just including the ID of the parent tab in the session save file is not enough - during startup, we then need to create a mapping between old and new tab IDs so we can fix up the parent tab IDs as well.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1301160 - Part 0 - Add an explicit string conversion to fix an error in Android Studio.

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

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1301160 - Part 1 - Include tab IDs in the session store data.

Having the previous tab ID in the session store data is required in order to be able to create a mapping between old and new IDs on starting up.
Likewise, the parent ID now needs to be tracked on the Gecko side as well, so it can be saved by the session store and later used when restoring a tab/session.

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

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1301160 - Part 2 - Update stored parent tab IDs during startup.

As long as the browser remains alive, tab IDs are stable and not reused, however if Firefox is killed and subsequently restarted, the ID sequence is reset and therefore all tabs opened through session restore will most probably be assigned a new tab ID.
Because of this, the stored parent tab IDs will be out of date and need to be refreshed as well so they continue pointing to the correct tab. To achieve that, we therefore create a mapping between old and new tab IDs as they are assigned and then use that to update the stored parent tab IDs, too.

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

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1301160 - Part 3 - Pass the saved parent tab ID when restoring tabs.

When restoring recently closed tabs during a browsing session, we can just pass the stored parentId as a parameter to addTab and be done with it.

During startup however, we first need to update the parent tab IDs before being able to use it.

At the moment, new tabs are always opened at the end of the tab list and cannot be resorted by the user, so child tabs always appear *after* their parent tabs in the tab list. However because we don't want to rely on this behaviour, we still need to collect a complete mapping between old and new tab IDs first before being able to update the stored parent tab IDs.

Therefore we cannot use the usual method of passing in the parentId during tab creation and have to set it after the fact for this case.

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

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1301160 - Part 4 - Test that the parent tab ID is properly restored when reopening a recently closed tab.

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

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824725 [details]
Bug 1301160 - Part 0 - Add an explicit string conversion to fix an error in Android Studio.

https://reviewboard.mozilla.org/r/102982/#review104236

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824726 [details]
Bug 1301160 - Part 1 - Include tab IDs in the session store data.

https://reviewboard.mozilla.org/r/102984/#review104238

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824727 [details]
Bug 1301160 - Part 2 - Update stored parent tab IDs during startup.

https://reviewboard.mozilla.org/r/102986/#review105244

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:314
(Diff revision 1)
> +            if (tabData == null) {
> +                return;
> +            }
> +

Right now this can never be null at this point, right?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1777
(Diff revision 1)
>                  windowObject.put("tabs", tabs);
> +                // ... and for recently closed tabs as well.
> +                JSONArray closedTabs = windowObject.optJSONArray("closedTabs");
> +                parser.updateParentId(closedTabs);
> +                windowObject.putOpt("closedTabs", closedTabs);
> +

We never clear tabIdMap. Is this a problem? I guess this map will never be really big - but if we don't need the values anymore after this point then we can just clear them, right?

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824728 [details]
Bug 1301160 - Part 3 - Pass the saved parent tab ID when restoring tabs.

https://reviewboard.mozilla.org/r/102988/#review104244

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:134
(Diff revision 1)
>              "Tab:StreamStart",
>              "Tab:StreamStop",
>              "Tab:AudioPlayingChange",
> -            "Tab:MediaPlaybackChange");
> +            "Tab:MediaPlaybackChange",
> +            "Tab:SetParentId",
> +            null);

Why do we introduce null here?

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8824729 [details]
Bug 1301160 - Part 4 - Test that the parent tab ID is properly restored when reopening a recently closed tab.

https://reviewboard.mozilla.org/r/102990/#review105258

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8824728 [details]
Bug 1301160 - Part 3 - Pass the saved parent tab ID when restoring tabs.

https://reviewboard.mozilla.org/r/102988/#review104244

> Why do we introduce null here?

That way, adding a new event at the bottom of the list will no longer require modifying the line above it as well to add a comma, so we'll get a cleaner blame history going forward. I copied this from what jchen did for the recent event conversions.

Comment 12

[Jan Henning [:JanH]]

Comment on attachment 8824727 [details]
Bug 1301160 - Part 2 - Update stored parent tab IDs during startup.

https://reviewboard.mozilla.org/r/102986/#review105244

> Right now this can never be null at this point, right?

Actually it can - closed tabs are not guaranteed to be present, and I'm using *opt*JSONArray to retrieve it (see further down in the diff). So if there aren't any closed tabs, the argument here *will* be null.

> We never clear tabIdMap. Is this a problem? I guess this map will never be really big - but if we don't need the values anymore after this point then we can just clear them, right?

Good point - I guess I've implicitly assumed that the parser will be alive during startup only. Looking a bit closer, the `LastSessionParser` is indeed instantiated [only once in a local variable in `restoreSessionTabs()`](https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1721), so everything should get GC'd afterwards, right?

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8824725 [details]
Bug 1301160 - Part 0 - Add an explicit string conversion to fix an error in Android Studio.

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

Comment 14

[Jan Henning [:JanH]]

Comment on attachment 8824726 [details]
Bug 1301160 - Part 1 - Include tab IDs in the session store data.

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

Comment 15

[Jan Henning [:JanH]]

Comment on attachment 8824727 [details]
Bug 1301160 - Part 2 - Update stored parent tab IDs during startup.

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

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 8824728 [details]
Bug 1301160 - Part 3 - Pass the saved parent tab ID when restoring tabs.

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

Comment 17

[Jan Henning [:JanH]]

Comment on attachment 8824729 [details]
Bug 1301160 - Part 4 - Test that the parent tab ID is properly restored when reopening a recently closed tab.

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

Comment 18

[Jan Henning [:JanH]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af1e1981ddc8
Part 0 - Add an explicit string conversion to fix an error in Android Studio. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/99c708631eee
Part 1 - Include tab IDs in the session store data. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/5fae18a086f5
Part 2 - Update stored parent tab IDs during startup. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/2b6c82ff1521
Part 3 - Pass the saved parent tab ID when restoring tabs. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/be590d43a999
Part 4 - Test that the parent tab ID is properly restored when reopening a recently closed tab. r=sebastian

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/af1e1981ddc8
https://hg.mozilla.org/mozilla-central/rev/99c708631eee
https://hg.mozilla.org/mozilla-central/rev/5fae18a086f5
https://hg.mozilla.org/mozilla-central/rev/2b6c82ff1521
https://hg.mozilla.org/mozilla-central/rev/be590d43a999