1333567 - Add-on install restart prompt looses open tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

At a guess this might be because during restart (https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/mobile/android/chrome/content/browser.js#5472) we're not sending "quit-application-proceeding" (https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/mobile/android/chrome/content/browser.js#1393), so the session store doesn't enter shutdown mode and flushes the result of the browser window closing (all tabs gone) to disk.

Comment 1

[Jan Henning [:JanH]]

It also seems like the event ordering during a restart is slightly different: I get a "domwindowclosed" notification *before* "application-quit", which triggers the session store's onWindowClose() code. Because the we haven't entered shutdown mode, we refresh the window data, which presumably causes the loss of all tabs from the session store data.

Once we send the required "quit-application-proceeding" notification beforehand however, this'll be harmless: The session store will have entered shutdown mode in that case, which means we won't run collectWindowData() until the final data flush, in which case the data of closed windows is simply ignored and preserved as is, so we won't loose any tabs.

Comment 2

[Jan Henning [:JanH]]

Attachment: Bug 1333567 - Part 0 - Simplfy shutdown data collection.

We've already got a function for collecting the session data from every browser window - we should use it instead of duplicating that code snippet over and over again.

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

Comment 3

[Jan Henning [:JanH]]

Attachment: Bug 1333567 - Send the notification expected by the session store when restarting, too.

Since bug 1228593, the mobile session store
- once again flushes its data when we are quitting, to make sure the latest state (including any potential cleaning of history/tabs) is flushed to disk
- ignores windows/tabs closing as a byproduct of shutdown

The latter point is dependent on a new shutdown notification introduced in that bug. Because we forgot to add that notification to the restart code used for add-on updates, in that case the session store currently doesn't enter shutdown mode and therefore records the window being closed during shutdown before flushing its data to disk, which means that all open tabs are lost.

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

Comment 4

[Jan Henning [:JanH]]

Comment on attachment 8830475 [details]
Bug 1333567 - Part 0 - Simplfy shutdown data collection.

Actually I've noticed that getCurrentState() also subsequently bundles up the window data to use it as a return value which isn't needed in those other cases, so scratch that and just fix the actual bug for now.

Comment 5

[Jan Henning [:JanH]]

Comment on attachment 8830476 [details]
Bug 1333567 - Send the notification expected by the session store when restarting, too.

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

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8830476 [details]
Bug 1333567 - Send the notification expected by the session store when restarting, too.

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

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8830476 [details]
Bug 1333567 - Send the notification expected by the session store when restarting, too.

https://reviewboard.mozilla.org/r/107224/#review109238

Comment 8

[Jan Henning [:JanH]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acb427e637c9
Send the notification expected by the session store when restarting, too. r=sebastian

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/acb427e637c9

Comment 11

[Jan Henning [:JanH]]

Comment on attachment 8830476 [details]
Bug 1333567 - Send the notification expected by the session store when restarting, too.

Approval Request Comment
[Feature/Bug causing the regression]: Mobile session store, bug 1228593
[User impact if declined]: Restarting when being prompted to after an add-on (un)install/update looses all tabs in the current session
[Is this code covered by automated tests?]: Probably not.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]:
1. Tab restore setting should be "Always restore".
2. Have some tabs open.
3. Install an add-on that requires a restart after installing (e.g. HTTPS Everywhere) and press the Restart button when prompted to.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This just adds a missing notification, so the shutdown notification sequence when restarting is the same as the one already used when quitting.
[String changes made/needed]: none

Requesting mozilla-release as well as a potential ride-along should we do another dot release.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, can your team help to verify this fix in nightly? Thanks!

Comment 13

[Ioana Chiorean]

We will have a look on Fennec side - Bogdan can you pls check this? Thanks

Comment 14

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - LG G4 (Android 6.0)
 - LG G4 (Android 5.1)
Build:
 - Nightly 54.0a1

Hello,

 I've verified the issue doing a install from addons.mozilla.org and manually installing the .xpi from the device. The tabs are properly restored after every install/uninstall + restart. Will needinfo myself to keep track of this as it rides the train.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 8830476 [details]
Bug 1333567 - Send the notification expected by the session store when restarting, too.

fennec session store fix for aurora53 and beta52

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8da1a1906fe6

Comment 17

[Carsten Book [:Tomcat]]

needs rebasing for beta

grafting 387384:8da1a1906fe6 "Bug 1333567 - Send the notification expected by the session store when restarting, too. r=sebastian, a=jcristau"
merging mobile/android/chrome/content/browser.js
merging mobile/android/components/SessionStore.js
warning: conflicts while merging mobile/android/components/SessionStore.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 18

[Jan Henning [:JanH]]

Attachment: Bug 1333567 - BETA.patch

Patch for Beta - jchen's EventDispatcher changes were conflicting.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/eaaf5c7cf8ec

Comment 20

[u549602]

Verified as fixed on 53.0a2(07-02-2017) and 52.0b4 .Tested using a Samsung Galaxy S6 EDGE (Android 6.0) and a Nexus 7 (Android 5.1.1)

Comment 21

[Sylvestre Ledru [:Sylvestre]]

needs rebasing also for release if we ever take it.

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8830476 [details]
Bug 1333567 - Send the notification expected by the session store when restarting, too.

We don't have then plan to have dot release for 51. Rel51-.