1231112 - Tab Groups are not migrated from Safe mode

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151208030212

If the update to Firefox 45 is made while using Firefox in Safe Mode, the tab groups are not migrated.

Steps to reproduce:
1. Install Firefox 44
2. Create several Panorama tab groups
3. Enter Safe mode: click on the Open Menu Button -> Open Help menu (?) -> Restart Firefox with Addons Disabled -> Start in Safe mode
4. Update to Firefox 45

Expected results:
1. The created Panorama tab groups are bookmarked in the Bookmarks Menu.
2. The Firefox-tabgroups-backup.json is generated.
3. The "Tab Groups are no more. Sorry." page is opened.

Actual results:
1. The created Panorama tab groups are not bookmarked in the Bookmarks Menu after the update.
2. The Firefox-tabgroups-backup.json is not generated.
3. The "Tab Groups are no more. Sorry." page is not opened.

Comment 1

[:Gijs (he/him)]

How are you updating from 44 (aurora) to 45 (still on Nightly)? Or are you using an old version of 44 nightly?

According to the steps, you're updating while in safe mode, is that right? In which case... is the first restart of 45 also in safe mode?

Comment 2

[Simona Badau, Desktop QA]

(In reply to :Gijs Kruitbosch from comment #1)
> How are you updating from 44 (aurora) to 45 (still on Nightly)? Or are you
> using an old version of 44 nightly?

The update is done on the Nightly channel, I used a 44.0a1 Nightly version. 

> According to the steps, you're updating while in safe mode, is that right?
> In which case... is the first restart of 45 also in safe mode?

Yes. The restart is also in safe mode.

Comment 3

[:Gijs (he/him)]

(In reply to Simona B from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > How are you updating from 44 (aurora) to 45 (still on Nightly)? Or are you
> > using an old version of 44 nightly?
> 
> The update is done on the Nightly channel, I used a 44.0a1 Nightly version. 
> 
> > According to the steps, you're updating while in safe mode, is that right?
> > In which case... is the first restart of 45 also in safe mode?
> 
> Yes. The restart is also in safe mode.

Are tabs migrated on the first restart outside of safe mode?

Comment 4

[Simona Badau, Desktop QA]

No. The tabs are not migrated. That is the problem: If the update to Nightly 45 is done while in safe mode there is no way to recover the panorama groups.

Comment 5

[:Gijs (he/him)]

Are the active group's tabs restored normally in the safe mode restart that's part of the upgrade?

Comment 6

[Simona Badau, Desktop QA]

Here is what I do:

1. Install the latest Nightly 44.0a1 from:
http://archive.mozilla.org/pub/firefox/nightly/2015/10/2015-10-29-03-02-58-mozilla-central/ 
2. Open Nightly 44 on a new profile where the update was unchecked
3. Create 3 panorama tab groups:
- first containing 3 websites opened from yahoo.com
- second containing 3 websites opened from nytimes.com
- third containing 3 websites opened from abcnews.com
4. Enter Safe mode: click on the Open Menu Button -> Open Help menu (?) -> Restart Firefox with Addons Disabled -> Start in Safe mode
5. Check that the panorama tab groups are available (all 3 of them)
6. Go to Help menu -> About Nightly -> Check for Updates -> Update to 45.0a1 -> Restart Nightly to update
7. The Nightly Safe Mode dialog is prompted giving me the possibility to Reset Nightly or Start in Safe Mode.
8. click on "Start in Safe Mode" button.

-> at this point Firefox 45 is launched having the 3 tabs from the Yahoo group opened + the first run one (https://www.mozilla.org/en-US/firefox/nightly/firstrun/?oldversion=44.0a1). The other 2 groups are not bookmarked, Firefox-tabgroups-backup.json is not generated, Tab Groups migration page is not opened

9. Exit Safe mode: click on the Open Menu Button -> Open Help menu (?) -> Restart Firefox with Addons Enabled 
10. Firefox is launched, no trace of the other 2 groups created in step 3.

Comment 7

[:Gijs (he/him)]

So this is essentially because in safe mode we show a modal dialog asking you whether you want to continue. That spins the event loop and in that time the session file gets read and its promise gets resolved, with the result that we dispatch the observer notification for that -- but migrateUI hasn't run yet (we're waiting for that dialog thing) and so we don't catch the notification and then everything is sad.

Specifically:

This dispatches the notification:

https://dxr.mozilla.org/mozilla-central/rev/a8965ae93c5d098a4f91ad9da72150bb43df07a7/browser/components/sessionstore/nsSessionStartup.js#127-132

called from:

https://dxr.mozilla.org/mozilla-central/rev/a8965ae93c5d098a4f91ad9da72150bb43df07a7/browser/components/sessionstore/nsSessionStartup.js#97-110

which runs off the observation of final-ui-startup, which also (!) runs this:

https://dxr.mozilla.org/mozilla-central/rev/a8965ae93c5d098a4f91ad9da72150bb43df07a7/browser/components/nsBrowserGlue.js#795-810

and here we see (lines 801-2) the modal dialog being fired...



Tim, should we just pull out this specific part of migrateUI to add the observer earlier? Is that too hacky? I'm scared about moving all of _migrateUI before we have the user make a decision about whether they continue in safe mode or reset firefox or whatever...

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

Comment 9

[:Gijs (he/him)]

Comment on attachment 8698039 [details]
MozReview Request: Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

Seems Tim is busy looking at rockets. Mike, do you feel comfortable reviewing this?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8698039 [details]
MozReview Request: Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

https://reviewboard.mozilla.org/r/27783/#review24963

::: browser/components/nsBrowserGlue.js:801
(Diff revision 1)
> +      // See https://bugzilla.mozilla.org/show_bug.cgi?id=1231112#c7 . We need to
> +      // register the observer early if we have to migrate tab groups
> +      let currentUIVersion = 0;
> +      try {
> +        currentUIVersion = Services.prefs.getIntPref("browser.migration.version");
> +      } catch(ex) {}
> +      if (currentUIVersion < 35) {
> +        this._maybeMigrateTabGroups();
> +      }

This doesn't feel right to me. I feel like we should probably only do this step once the user has closed the modal dialog / made their choice about how to proceed.

So if I understand correctly, in the bad case, the session file is getting read while this dialog is open because the nsSessionStartup component is registered to init for the app-startup category.

Is it possible to modify the nsISessionStartup interface to expose a read-only property that returns true if the session file has been read in yet? If so, then in \_maybeMigrateTabGroups, we can check for that and skip setting the observer.

How does that sound?

Comment 11

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> Comment on attachment 8698039 [details]
> MozReview Request: Bug 1231112 - work around tab groups migration issue in
> safe mode, r?ttaubert
> 
> https://reviewboard.mozilla.org/r/27783/#review24963
> 
> ::: browser/components/nsBrowserGlue.js:801
> (Diff revision 1)
> > +      // See https://bugzilla.mozilla.org/show_bug.cgi?id=1231112#c7 . We need to
> > +      // register the observer early if we have to migrate tab groups
> > +      let currentUIVersion = 0;
> > +      try {
> > +        currentUIVersion = Services.prefs.getIntPref("browser.migration.version");
> > +      } catch(ex) {}
> > +      if (currentUIVersion < 35) {
> > +        this._maybeMigrateTabGroups();
> > +      }
> 
> This doesn't feel right to me. I feel like we should probably only do this
> step once the user has closed the modal dialog / made their choice about how
> to proceed.

That is what currently happens, fwiw. The rest of the code will run to completion irrespective of your choice in the dialog, I expect.

> So if I understand correctly, in the bad case, the session file is getting
> read while this dialog is open because the nsSessionStartup component is
> registered to init for the app-startup category.

Correct. Note that the observer gets the contents of the session file as an nsISupportsString and the purpose of that observer notification is to let consumers modify it, which this observer will do.

> Is it possible to modify the nsISessionStartup interface to expose a
> read-only property that returns true if the session file has been read in
> yet? If so, then in \_maybeMigrateTabGroups, we can check for that and skip
> setting the observer.

That doesn't help - the observer actually modifies the contents of the session restore file. That's its point. I don't know that there is a way to do this after the fact. Am I missing something?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Oh I see - so by the time the file is read, we're too late. I understand.

I ... I don't think I'm the right person to review this, actually. Early start-up of SessionStore is too unfamiliar. I'll follow this bug along though, so I can learn. ttaubert might be your best bet.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8698039 [details]
MozReview Request: Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

OK, redirected back to Tim for when he's back from running around the US looking at rockets ;-)

I mean, I think it's essentially this patch (with nits/modifications, perhaps) or accept that we can't make it work in safe mode. A general (ie not hacky, which this definitely is) solution is really difficult, changing the notification we use for session restore to hang off something else we dispatch after showing this dialog will have add-on impact and require a large-ish set of changes, while if we want to do this it'll have to be for 45 so it'll need aurora uplift...

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8698039 [details]
MozReview Request: Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

https://reviewboard.mozilla.org/r/27783/#review26279

While this is indeed a little hacky I think this is the best possible solution for now. I wouldn't invest too much time as the code disappears in a few releases. Migrating before the modal safe mode dialog is shown doesn't seem problematic, it's not like the user has a choice anyway... We ripped out Tab Groups so it's going to hurt sooner or later and migrating earlier doesn't seem to hurt.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f3a755506d33

Comment 16

[:Gijs (he/him)]

Comment on attachment 8698039 [details]
MozReview Request: Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

Approval Request Comment
[Feature/regressing bug #]: tab groups removal
[User impact if declined]: tab groups migration doesn't work in safe mode
[Describe test coverage new/current, TreeHerder]: not for this code path :-(
[Risks and why]: low-ish. Specific change to accommodate this migration in safe mode only. Shouldn't affect anything else
[String/UUID change made/needed]: no

Comment 17

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

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8698039 [details]
MozReview Request: Bug 1231112 - work around tab groups migration issue in safe mode, r?ttaubert

Polish the removal of tab group, taking it.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/05306fe44f6f

Comment 20

[Brindusa Tot, Desktop QA]

Verified on Nightly build on Windows and Ubuntu:
 - Windows 7x64 - 46.0a1 - Build ID 20160107030235, Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
 - Ububtu 14.04 x32 - 46.0a1 - Build ID 20160108030334, Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0

Comment 21

[ovidiu boca[:Ovidiu]]

 Build ID 	20160110030214
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:46.0) Gecko/20100101 Firefox/46.0

Verified on Nightly 46.0a1 build on Mac OS X 10.10 I can reproduce the problem, tab groups are not migrated in Safe mode.
Verified on DevelopersEdition 45.0a2 on Mac OS X 10.10 I can reproduce the problem, tab groups are not migrated in Safe mode.

Comment 22

[:Gijs (he/him)]

(In reply to ovidiu boca from comment #21)
>  Build ID 	20160110030214
> User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:46.0)
> Gecko/20100101 Firefox/46.0
> 
> Verified on Nightly 46.0a1 build on Mac OS X 10.10 I can reproduce the
> problem, tab groups are not migrated in Safe mode.
> Verified on DevelopersEdition 45.0a2 on Mac OS X 10.10 I can reproduce the
> problem, tab groups are not migrated in Safe mode.

Can you post your exact steps as a separate bug? There is no reason why OSX should be special here. Because we're tracking uplifts, it also makes more sense to open a separate bug.

I'm going to re-resolve this bug so we can track this fix correctly.

Comment 23

[ovidiu boca[:Ovidiu]]

Hi,
Retested on Nightly and Aurora and it seems that the problem was profile dependent. Not reproducible anymore, verified as fixed on Mac OS X 10.10:

Aurora: 45.0a2 (2016-01-11) 
Nightly: 46.0a1 (2016-01-11)

Comment 24

[:Gijs (he/him)]

(In reply to ovidiu boca from comment #23)
> Hi,
> Retested on Nightly and Aurora and it seems that the problem was profile
> dependent. Not reproducible anymore, verified as fixed on Mac OS X 10.10:
> 
> Aurora: 45.0a2 (2016-01-11) 
> Nightly: 46.0a1 (2016-01-11)

Thanks!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to ovidiu boca from comment #23)
> it seems that the problem was profile dependent

Any idea what the problem was with the profile? Is this something that would likely bite users in the wild or just being in a bad state from testing?

Comment 26

[ovidiu boca[:Ovidiu]]

I used this scenario:
Install Nightly 46 and make a new profile 
Install Nightly 44 and start it with the profile made on Nightly 46, than proceeded on upgrading/migrating to Nightly 46 (panorama, bookmarks).


Is sort of related to Bug 1231629. 

In my opinion I think the users won't be affected by this because this scenario is not very common.