1282830 - Closing a zombie tab doesn't trigger a session save

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

For whatever reasons, when closing a zombie tab we're exiting early from this onTabRemove here (https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/mobile/android/components/SessionStore.js#466) and never trigger a session save.
This means that if Firefox is subsequently closed without anything else having triggered a session save, those tabs will in fact reappear the next time Firefox is started.

It's also an alternative way for bug 1256277 to have happened, which coincidentally is also not as timing-dependant as my original STR there.

Comment 1

[Jan Henning [:JanH]]

Attachment: Bug 1282830 - Trigger session saves when closing zombie tabs, too.

A tab being in a delay-loaded "zombie" state or not shouldn't have any influence on the behaviour of onTabRemove - since we remove it from the session store's sphere of influence, its __SS_data can be safely deleted anyway and whether or not a session save needs to be triggered should depend only on the aNoNotfication parameter passed by the caller.

Otherwise, with the current behaviour, the fact that those tabs have been closed will not get saved to disk if no subsequent session save is triggered through any other means (e.g. selecting a different tab, scrolling, ...) before closing Firefox.

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

Comment 2

[:Margaret Leibovic]

Comment on attachment 8766476 [details]
Bug 1282830 - Trigger session saves when closing zombie tabs, too.

Sorry I've been so slow to get around to reviews, redirecting to Sebastian.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8766476 [details]
Bug 1282830 - Trigger session saves when closing zombie tabs, too.

https://reviewboard.mozilla.org/r/61340/#review59708

Sounds reasonable.

I was trying to track back why this line existed in the first place. It seems like it was added in this changeset from 2010 (bug 617851):
https://hg.mozilla.org/mozilla-central/diff/e6c70153e799/mobile/components/SessionStore.js#l1.125
However I do not see any particular reason why we might still need it, so let's see how it goes.

Comment 4

[Jan Henning [:JanH]]

Yup, I had already looked at that commit, too, but was left none the wiser as to why that line was there.

Comment 5

[Jan Henning [:JanH]]

Comment on attachment 8766476 [details]
Bug 1282830 - Trigger session saves when closing zombie tabs, too.

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

Comment 6

[Jan Henning [:JanH]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5fce05217908
Trigger session saves when closing zombie tabs, too. r=sebastian

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fce05217908

Comment 10

[Jan Henning [:JanH]]

Comment on attachment 8766476 [details]
Bug 1282830 - Trigger session saves when closing zombie tabs, too.

Approval Request Comment
[Feature/regressing bug #]: mobile session store
[User impact if declined]: closing a zombified background tab won't trigger a session save, so if Firefox is subsequently closed/killed, those tabs might reappear on next startup
[Describe test coverage new/current, TreeHerder]: manual, some time on Nightly
[Risks and why]: low, sooner or later (after switching to a different tab, opening a new tab, closing a non-zombified tab, scrolling, page navigation, ...) a session save would have been triggered anyway, so we just make sure to immediately trigger one after closing a zombified tab, too
[String/UUID change made/needed]: none

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8766476 [details]
Bug 1282830 - Trigger session saves when closing zombie tabs, too.

Less zombies is good, taking it!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7cb293390a5f