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

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

unspecified
Firefox 53
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
... 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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1329330
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
Attachment #8824725 - Flags: review?(s.kaspari) → review+
Comment on attachment 8824726 [details]
Bug 1301160 - Part 1 - Include tab IDs in the session store data.

https://reviewboard.mozilla.org/r/102984/#review104238
Attachment #8824726 - Flags: review?(s.kaspari) → review+
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?
Attachment #8824727 - Flags: review?(s.kaspari) → review+
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?
Attachment #8824728 - Flags: review?(s.kaspari) → review+
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
Attachment #8824729 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 11

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 12

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Blocks: 1338899
You need to log in before you can comment on or make changes to this bug.