1415133 - Downgrading Firefox 57 to 52 ESR loses bookmarks

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Jim informs me that the a11y team plans to prompt a small percentage of users with unsupported configurations in 57 to switch to 52 ESR instead (temporarily?). Downgrading versions in the same profile is unsupported, but this specific case may merit some additional work. 

One thing we discovered is that bookmarks are lost if a user in a new profile created with 57 switches to 52 ESR. Old profiles upgrading to 57 and then downgrading to 52 ESR might not face this issue. Marco is doing the investigation.

Comment 1

[Marco Bonardo [:mak]]

I must note the loss is only visual, re-upgrading has no loss. Not that it actually matters much for the user.
Downgrading a profile Created in Firefox 55 or greater to 52 would lose bookmarks.
Downgrading a profile upgraded to Firefox 58 or greater to 52 would lose bookmarks.
The other cases should be fine.
What we can do, is issue a special patch to ESR52 that will consider corrupt a database whose schema version is greater than the one used in 55. A corrupt database is thrown away and regenerated on startup. Bookmarks are restored from the latest bookmarks backup, history is lost.

Comment 2

[Marco Bonardo [:mak]]

Attachment: ESR52 patch

Comment 3

[Marco Bonardo [:mak]]

There were 2 schema versions in 55: v36 and v37. The one with downgrade issues is v37, because it removed the moz_favicons table from new profiles.
Firefox 58 did the same also for old profiles.
Thus the patch picks versions > 36.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8925881 [details] [diff] [review]
ESR52 patch

Review of attachment 8925881 [details] [diff] [review]:
-----------------------------------------------------------------

This is safe enough for ESR.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Issues for users migrating from 57 to ESR52.

We're prompting some users to do this migration, so I think this needs to be in 52.5.0.

Comment 6

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

OK, can you request uplift? I need to start the ESR build today.

Comment 7

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

Don't we say in many places that downgrading with the same profile is unsupported? I remember seeing many bugs on this over time. Shouldn't we test this downgrade path better before recommending that users do it?

Comment 8

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

I see the tests now and it looks like creating a new bookmark doesn't work after a downgrade.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8925881 [details] [diff] [review]
ESR52 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Downgrading any version from 55 on to 52 causes the Bookmarks to not appear (and likely break the Address Bar). This is particularly important when we suggest users to move from 57 to ESR52 for a11y reasons.
User impact if declined: Bookmarks, history and Address Bar broken
Fix Landed on Version: this fix is ESR52 only
Risk to taking this patch (and alternatives if risky): The patch is trivial, it will just cause Places to consider the database corrupt if its schema version is too much in the future. On such corruption Places throws away the db and creates a new one, importing bookmarks from the last backup.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

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

Comment on attachment 8925881 [details] [diff] [review]
ESR52 patch

Fix some issues with bookmarks for downgrade path to ESR. Taking this for build 2 of 52.5.0.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr52/rev/22fe3ff3f923

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Is there anything else to do here or can we resolve this bug?

Comment 13

[Marco Bonardo [:mak]]

The bug doesn't need to land elsewhere, so we can mark it as fixed.
QE must still happen.

Comment 14

[Alexandru Simonca [:asimonca], Release Desktop QA]

Hi,
I have tested this issue over the past few days and everytime I lose my bookmarks. I have used the 57.0 RC  and downgraded it to 52.5.0ESR. The only way I can keep the bookmarks after a downgrade is if I have the sync account set up and synced. 
You can see a screencast of this issue here: https://goo.gl/vQdzWb

Is there something I'm missing?

Comment 15

[Marco Bonardo [:mak]]

Could you please check if in the Library / Import & Save / Restore you have a bookmarks backup listed?
Did you create those bookmarks just before upgrading? Maybe a bookmarks backup was not created yet.
Could you please also check if in the profile folder you have a places.sqlite-corrupt and what's in the bookmarkBackups folder?

For me, before this patch, the bookmarks toolbar was appearing completely empty as well as the Library and the Location Bar was not working.

Comment 16

[Jim Mathies [:jimm]]

Do you have to manually trigger a backup? I've a fresh install of 56 on Win10 which I've opened and closed a few times, saved some bookmarks, etc.. I keep checking the 'bookmarkbackup' profile folder but it remains empty.

Comment 17

[Jim Mathies [:jimm]]

Interesting, in 56 I did a manual backup which I saved to my desktop. This in turn triggered a backup that was saved to the backup folder.

Comment 18

[Marco Bonardo [:mak]]

(In reply to Jim Mathies [:jimm] from comment #16)
> Do you have to manually trigger a backup? I've a fresh install of 56 on
> Win10 which I've opened and closed a few times, saved some bookmarks, etc..
> I keep checking the 'bookmarkbackup' profile folder but it remains empty.

The backups happen on idle-daily.
As you discovered, if you manually store a json file, we also copy it to the backups folder.

Comment 19

[Marco Bonardo [:mak]]

I must add, we backup on idle daily, then if we couldn't hit an idle for 3 days, we enforce it on shutdown.

Comment 20

[Jim Mathies [:jimm]]

This is good news! I just did the following test:

Win10, latest updates
1) download 56, 57, and 'build 2' esr
2) install 56 (default install)
3) ioen 56 and display bookmarks toolbar, add some bookmarks
4) invoke a manual bookmarks backup and discard the backup json file I saved to the desktop
5) closed 56
6) install 57 (default install)
7) launch 57

result: bookmarks from 56 are present, bookmarks toolbar is visible, adding a bookmark in 57 works

8) close 57
9) install esr 52 build 2 (default install)
10 launch esr 52

result: bookmarks toolbar state, and all bookmarks in 56 restored. adding a bookmark to the toolbar works. Note, the one bookmark I saved in 57 did not come back (because it wasn't in the backup json file from 56).

Assuming users have a backup json, and I think it's safe to say most users who have been running 56 for a while should, I think we're in ok shape here.

Comment 21

[Alexandru Simonca [:asimonca], Release Desktop QA]

How much time does the browser have to be in idle to trigger the backup?

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Is there any way we could force-trigger a bookmarks backup to run if a user gets the JAWS ESR52 downgrade prompt?

Comment 23

[Marco Bonardo [:mak]]

(In reply to Alexandru Simonca, QA (:asimonca) from comment #21)
> How much time does the browser have to be in idle to trigger the backup?

We start from 8 minutes of idle, then if in the last 24 hours we didn't hit it, we reduce it to 4 minutes of idle. If after 3 days we didn't hit a 4 minutes idle yet, we just force a backup on shutdown.
Or you can trigger one manually as Jimm explained (From the Library / Import and Save / Save)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Is there any way we could force-trigger a bookmarks backup to run if a user
> gets the JAWS ESR52 downgrade prompt?

I don't know the code for the downgrade, but this is a good code example to do a backup:
http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/browser/components/nsBrowserGlue.js#1683

Though, imo it's not necessary, the worst case is someone that created a new profile less than 3 days from the downgrade and never hit a long enough idle.

Comment 24

[Alexandru Simonca [:asimonca], Release Desktop QA]

I confirm Jim's steps work. I did the same thing (with the manual backup) and the json file appeared in the bookmarkbackup folder and then did the downgrade and my bookmarks were there. 
I think we're good to go with this one.
Marking as Verified Fixed.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Marco Bonardo [::mak] from comment #23)
> Though, imo it's not necessary, the worst case is someone that created a new
> profile less than 3 days from the downgrade and never hit a long enough idle.

I agree that it's a bit of a contrived and unlikely scenario, but if it's easy enough to implement and low-risk, maybe it'd be worth doing out of an abundance of caution.

Comment 26

[Marco Bonardo [:mak]]

I don't think it's risk free, the backup process takes some seconds and it's async, if it happens at the wrong time it may cause more issues than benefits. The downgrade process looks complex enough by itself. But as I said, I don't know much about that process.

Comment 27

[Andreas Jung]

I can also confirm this bug is fixed, but i found bug 1416506 in the process.

Comment 28

[John Kaufmann]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> (In reply to Marco Bonardo [::mak] from comment #23)
> > Though, imo it's not necessary, the worst case is someone that created a new
> > profile less than 3 days from the downgrade and never hit a long enough idle.
> 
> I agree that it's a bit of a contrived and unlikely scenario, but if it's
> easy enough to implement and low-risk, maybe it'd be worth doing out of an
> abundance of caution.

Unlikely, probably; contrived, no: I just did it. Solution was to delete the last bookmarks backup (easily identified by small size as well as by timestamp), then restart ESR52 with earlier places.sqlite.* files from backup. [Still lost history, but the bookmarks are more valuable, especially with the relevant history otherwise saved by Session Manager.]  That last bookmarks backup may be what confounded Alexandru's early attempts (comments #14,#16). 

Instead of just taking the most recent backup, could the bookmarks restore look at the last 2-most-recent (or 3-most-recent) files and apply a file-size test (say, they differ by {1|2|3} orders of magnitude) to pick the one with content?  In the context of limited resources and a mostly-working fix, I hate to bring this up, but I still see a lot of recent posts by people with this downgrade loss-of-bookmarks problem. Maybe they should have a sync account, as Alexandru said, but that is still the exception. With so many people reacting to the loss of important extensions by temporarily downgrading to ESR to recover them, maybe it would make sense to make that process a little less painful -- if it's not too painful to do the recommended check.