Closed Bug 1333567 Opened 3 years ago Closed 3 years ago
Add-on install restart prompt looses open tabs
59 bytes, text/x-review-board-request
2.82 KB, patch
|Details | Diff | Splinter Review|
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+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/acb427e637c9 Send the notification expected by the session store when restarting, too. r=sebastian
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.
Andrei, can your team help to verify this fix in nightly? Thanks!
We will have a look on Fennec side - Bogdan can you pls check this? Thanks
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.
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
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')
Patch for Beta - jchen's EventDispatcher changes were conflicting.
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-
You need to log in before you can comment on or make changes to this bug.