Closed Bug 1333567 Opened 3 years ago Closed 3 years ago

Add-on install restart prompt looses open tabs

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox51 --- fix-optional
firefox52 --- verified
firefox53 --- verified
firefox54 --- verified

People

(Reporter: JanH, Assigned: JanH)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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 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.
Attachment #8830475 - Flags: review?(s.kaspari)
Attachment #8830475 - Attachment is obsolete: true
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
Attachment #8830476 - Flags: review?(s.kaspari) → review+
Blocks: 743662
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/acb427e637c9
Send the notification expected by the session store when restarting, too. r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/acb427e637c9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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.
Attachment #8830476 - Flags: approval-mozilla-release?
Attachment #8830476 - Flags: approval-mozilla-beta?
Attachment #8830476 - Flags: approval-mozilla-aurora?
Andrei, can your team help to verify this fix in nightly? Thanks!
Flags: needinfo?(andrei.vaida)
We will have a look on Fennec side - Bogdan can you pls check this? Thanks
Flags: needinfo?(andrei.vaida)
QA Contact: bogdan.surd
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.
Flags: needinfo?(bogdan.surd)
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
Attachment #8830476 - Flags: approval-mozilla-beta?
Attachment #8830476 - Flags: approval-mozilla-beta+
Attachment #8830476 - Flags: approval-mozilla-aurora?
Attachment #8830476 - Flags: approval-mozilla-aurora+
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')
Flags: needinfo?(jh+bugzilla)
Patch for Beta - jchen's EventDispatcher changes were conflicting.
Flags: needinfo?(jh+bugzilla)
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)
needs rebasing also for release if we ever take it.
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-.
Attachment #8830476 - Flags: approval-mozilla-release? → approval-mozilla-release-
Flags: needinfo?(bogdan.surd)
You need to log in before you can comment on or make changes to this bug.