Closed
Bug 1333567
Opened 8 years ago
Closed 8 years ago
Add-on install restart prompt looses open tabs
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox51 fix-optional, firefox52 verified, firefox53 verified, firefox54 verified)
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)
(deleted),
text/x-review-board-request
|
sebastian
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
gchang
:
approval-mozilla-release-
|
Details |
(deleted),
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.
Assignee | ||
Comment 1•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8830475 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 7•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•8 years ago
|
||
Keywords: checkin-needed
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
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Assignee | ||
Comment 11•8 years ago
|
||
str |
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)
Comment 13•8 years ago
|
||
We will have a look on Fennec side - Bogdan can you pls check this? Thanks
Flags: needinfo?(andrei.vaida)
QA Contact: bogdan.surd
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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+
Comment 16•8 years ago
|
||
bugherder uplift |
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
Patch for Beta - jchen's EventDispatcher changes were conflicting.
Flags: needinfo?(jh+bugzilla)
Comment 19•8 years ago
|
||
bugherder uplift |
Comment 20•8 years ago
|
||
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•8 years ago
|
||
needs rebasing also for release if we ever take it.
Comment 22•8 years ago
|
||
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-
Updated•8 years ago
|
Flags: needinfo?(bogdan.surd)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•