Verdi [:verdi] Best to slack me Reporter
	Description • 8 years ago

I set Chrome as my default browser in Windows (with just the default, no-name user profile). I bookmarked the New York Times and Twitter. I also signed in to Twitter and saved my password. Then, leaving Chrome open, I installed Firefox 50 beta 1 and encountered some unexpected things:
1. Two Firefox tabs were open and the active tab was https://www.mozilla.org/en-US/firefox/windows-10/welcome/ - My expectation was that only about:home would be displayed.
2. I got the refresh bottom infobar prompt. I had deleted all copies of Firefox and all Firefox profiles so there shouldn't have been anything to refresh. This seems unrelated to automigration.
3. Although my bookmarks were imported, I didn't see that my history, passwords or form data was imported. I expected that they would be imported along with my bookmarks.

Here's a video of what I saw: https://www.dropbox.com/s/whv406jm7108akv/automigration-test.mp4?dl=0

	:Gijs (he/him) Assignee
	Comment 1 • 8 years ago

(In reply to Verdi [:verdi] from comment #0)
> I set Chrome as my default browser in Windows (with just the default,
> no-name user profile). I bookmarked the New York Times and Twitter. I also
> signed in to Twitter and saved my password. Then, leaving Chrome open, I
> installed Firefox 50 beta 1 and encountered some unexpected things:
> 1. Two Firefox tabs were open and the active tab was
> https://www.mozilla.org/en-US/firefox/windows-10/welcome/ - My expectation
> was that only about:home would be displayed.

Dolske and I talked about this and, if I remember correctly, said that the Windows 10 page wasn't really worth worrying about. It's separate from the other whatsnew pages and also shows up if you have an existing profile and update your machine to Windows 10, so disabling it in beta would have had consequences beyond "first run". ISTR there's a bug on file for removing it altogether. The other tab is about:home so this is expected behaviour.

> 2. I got the refresh bottom infobar prompt. I had deleted all copies of
> Firefox and all Firefox profiles so there shouldn't have been anything to
> refresh. This seems unrelated to automigration.

bug 1290495, which you already filed a while back. Also note that you yourself filed the bug that got this feature implemented in bug 1095739. :-\

> 3. Although my bookmarks were imported, I didn't see that my history,
> passwords or form data was imported. I expected that they would be imported
> along with my bookmarks.

It's known that importing history doesn't work while Firefox is running if you're testing on 50 beta (bug 1285041). It'll work on Nightly (but the pref is turned off there, so no testing).

We don't have a form data importer for any browser except Firefox so automigration doesn't ask for that data (and if it did, would never get any), see https://bugzilla.mozilla.org/show_bug.cgi?id=1271799#c3 (which you're CC'd on).

> Here's a video of what I saw:
> https://www.dropbox.com/s/whv406jm7108akv/automigration-test.mp4?dl=0

When this happens, are there errors in the browser console on that initial run? It doesn't look like you opened it. I imagine we just need to apply the same code we're using for history to the passwords (and/or cookies) db reads in that file.

Note that password import from Chrome is only implemented on Windows.

	:Gijs (he/him) Assignee
	Comment 2 • 8 years ago

(In reply to :Gijs Kruitbosch from comment #1)
>  ISTR there's a bug on file for removing [the windows 10 page]
> altogether.

bug 1274633.

	Verdi [:verdi] Best to slack me Reporter
	Comment 3 • 8 years ago

(In reply to :Gijs Kruitbosch from comment #1)
> (In reply to Verdi [:verdi] from comment #0)
> > I set Chrome as my default browser in Windows (with just the default,
> > no-name user profile). I bookmarked the New York Times and Twitter. I also
> > signed in to Twitter and saved my password. Then, leaving Chrome open, I
> > installed Firefox 50 beta 1 and encountered some unexpected things:
> > 1. Two Firefox tabs were open and the active tab was
> > https://www.mozilla.org/en-US/firefox/windows-10/welcome/ - My expectation
> > was that only about:home would be displayed.
> 
> Dolske and I talked about this and, if I remember correctly, said that the
> Windows 10 page wasn't really worth worrying about. It's separate from the
> other whatsnew pages and also shows up if you have an existing profile and
> update your machine to Windows 10, so disabling it in beta would have had
> consequences beyond "first run". ISTR there's a bug on file for removing it
> altogether. The other tab is about:home so this is expected behaviour.
> 

I remember agreeing that we would remove all pages and start people on about:home. That's not a test of the actual experience we want to build but it's certainly better than starting people on this welcome page. The bug to remove this page has been approved https://bugzilla.mozilla.org/show_bug.cgi?id=1274633#c7 so it's fine if there are consequences beyond first run - they're intended.


> > 2. I got the refresh bottom infobar prompt. I had deleted all copies of
> > Firefox and all Firefox profiles so there shouldn't have been anything to
> > refresh. This seems unrelated to automigration.
> 
> bug 1290495, which you already filed a while back. Also note that you
> yourself filed the bug that got this feature implemented in bug 1095739. :-\
> 

Right. Thanks for the reminder - I forgot that I filed Bug 1290495. This is a consequence of implementing Bug 1095739 without any of the other intended work. 


> > 3. Although my bookmarks were imported, I didn't see that my history,
> > passwords or form data was imported. I expected that they would be imported
> > along with my bookmarks.
> 
> It's known that importing history doesn't work while Firefox is running if
> you're testing on 50 beta (bug 1285041). It'll work on Nightly (but the pref
> is turned off there, so no testing).
> 

Why didn't we uplift this for the test?


> We don't have a form data importer for any browser except Firefox so
> automigration doesn't ask for that data (and if it did, would never get
> any), see https://bugzilla.mozilla.org/show_bug.cgi?id=1271799#c3 (which
> you're CC'd on).
> 

I missed that. I assumed we were doing this since it's what we agreed upon https://bugzilla.mozilla.org/show_bug.cgi?id=1271774#c2 Is there no way to ever make that work?


> > Here's a video of what I saw:
> > https://www.dropbox.com/s/whv406jm7108akv/automigration-test.mp4?dl=0
> 
> When this happens, are there errors in the browser console on that initial
> run? It doesn't look like you opened it. I imagine we just need to apply the
> same code we're using for history to the passwords (and/or cookies) db reads
> in that file.
> 
> Note that password import from Chrome is only implemented on Windows.

I didn't look the first time. I repeated the procedure (after uninstalling and removing the profile) and attached the results.

	:Gijs (he/him) Assignee
	Comment 4 • 8 years ago

(In reply to Verdi [:verdi] from comment #3)
> Created attachment 8795437 [details]
> browser-console.png
> 
> (In reply to :Gijs Kruitbosch from comment #1)
> > (In reply to Verdi [:verdi] from comment #0)
> > > I set Chrome as my default browser in Windows (with just the default,
> > > no-name user profile). I bookmarked the New York Times and Twitter. I also
> > > signed in to Twitter and saved my password. Then, leaving Chrome open, I
> > > installed Firefox 50 beta 1 and encountered some unexpected things:
> > > 1. Two Firefox tabs were open and the active tab was
> > > https://www.mozilla.org/en-US/firefox/windows-10/welcome/ - My expectation
> > > was that only about:home would be displayed.
> > 
> > Dolske and I talked about this and, if I remember correctly, said that the
> > Windows 10 page wasn't really worth worrying about. It's separate from the
> > other whatsnew pages and also shows up if you have an existing profile and
> > update your machine to Windows 10, so disabling it in beta would have had
> > consequences beyond "first run". ISTR there's a bug on file for removing it
> > altogether. The other tab is about:home so this is expected behaviour.
> > 
> 
> I remember agreeing that we would remove all pages and start people on
> about:home. That's not a test of the actual experience we want to build

We were all agreed that that was not possible anyway. We're not testing what we ultimately want to build. There are many, many ways in which this isn't the thing we "want to build", but this is what we could do for Firefox 50, and this will give us an idea what to focus on when we move forward.

I don't think the extra page makes a meaningful difference to the experiment. If you think it does and you think we shouldn't have gone to beta with it turned on, I'm sorry. We can compare and contrast the win10 results with Win8 to see what the difference is like. I would prefer not to change things now (for 50) because it will make the data-analysis harder. I'll see about writing the patch to remove the pages on that bug entirely (rather than pref off, which is what happened with firstrun) later this week.

> > > 3. Although my bookmarks were imported, I didn't see that my history,
> > > passwords or form data was imported. I expected that they would be imported
> > > along with my bookmarks.
> > 
> > It's known that importing history doesn't work while Firefox is running if
> > you're testing on 50 beta (bug 1285041). It'll work on Nightly (but the pref
> > is turned off there, so no testing).
> > 
> 
> Why didn't we uplift this for the test?

It requires an update to sqlite, which only landed in 51. If we wanted to uplift we would have had to uplift that, too. So I don't think it should be uplifted to 50.

Separately, because getting everything uplifted initially happened for the 49 experiment, and at that time this patch wasn't ready. When the 49 thing was done and I started work on the chrome stuff, and simultaneously looked at what was required to move forward with running our beta experiment, I didn't think fixing the Chrome issue was a necessity and/or that uplifting it was going to be feasible, and preferred "going to beta with the code I had", rather than waiting for 51 or uplifting.

> > We don't have a form data importer for any browser except Firefox so
> > automigration doesn't ask for that data (and if it did, would never get
> > any), see https://bugzilla.mozilla.org/show_bug.cgi?id=1271799#c3 (which
> > you're CC'd on).
> > 
> 
> I missed that. I assumed we were doing this since it's what we agreed upon
> https://bugzilla.mozilla.org/show_bug.cgi?id=1271774#c2 Is there no way to
> ever make that work?

Not unless someone writes the form data importers, and nobody has. I don't know if that's difficult or not. I also don't know that it would be particularly valuable.

We've never imported form data - nothing has changed compared to a manual import. I listed the datatype in the comment you cited in the abstract sense that it's one of the datatypes that the migration interface support, just like 'settings' (which we only support for Safari and IE - though tbh I think we should just kill it completely) and passwords (which we only support on some versions of IE, Edge, and Windows Chrome - but not on OS X or Linux Chrome, or (IIRC) Safari, or ...). For 360se, we only support bookmarks, not even history or passwords. We can't import almost anything from Edge while it's running, and there's no way to fix that that I'm aware of. All of this was known beforehand, and expanding the scope of the importers wasn't a goal for this project, certainly not for the beta experiment.

In case it's helpful, at the time of the 49 thing, I did a write-up of considerations I had for the beta uplift: https://docs.google.com/document/d/1chaGoLCo1eZiJ784CMG8XYV6QW2dzaFBZwhDeRhNjVw/edit#heading=h.klnsvtx64fn6 .

> I didn't look the first time. I repeated the procedure (after uninstalling
> and removing the profile) and attached the results.

Thanks. So my guess was correct, calling openUnsharedDatabase throws if Chrome is running.

	Justin Dolske [:Dolske]
	Comment 5 • 8 years ago

(In reply to Verdi [:verdi] from comment #3)

> I remember agreeing that we would remove all pages and start people on
> about:home. That's not a test of the actual experience we want to build but
> it's certainly better than starting people on this welcome page. The bug to
> remove this page has been approved
> https://bugzilla.mozilla.org/show_bug.cgi?id=1274633#c7 so it's fine if
> there are consequences beyond first run - they're intended.

The sole reason for fiddling with the first-run pages for the current test was that we can't offer to undo the automigration once Sync is enabled, and the current pages encouraged people to do that. The Windows 10 page does not (at least not as an in-your-face CTA), so it was left alone.

I didn't realize bug 1274633 had been cleared. It's actually a little surprising, because it was an effort to get that same set of people to signoff on just temporarily disabling those pages on beta for this experiment. So seems everyone missed this!


> > It's known that importing history doesn't work while Firefox is running if
> > you're testing on 50 beta (bug 1285041). It'll work on Nightly (but the pref
> > is turned off there, so no testing).
> > 
> 
> Why didn't we uplift this for the test?

As Gijs noted, this was a significant code change, and we already were having pushback from rel-mgmt on enabling this for Beta. As it was, we barely had time to get things enabled for the first beta (Gijs had to land the pref flip on a weekend).

	Justin Dolske [:Dolske]
	Comment 7 • 8 years ago

(In reply to Justin Dolske [:Dolske] from comment #5)

> > I remember agreeing that we would remove all pages and start people on
> > about:home. That's not a test of the actual experience we want to build but
> > it's certainly better than starting people on this welcome page. The bug to
> > remove this page has been approved
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1274633#c7 so it's fine if
> > there are consequences beyond first run - they're intended.
> 
[...]
> 
> I didn't realize bug 1274633 had been cleared. It's actually a little
> surprising, because it was an effort to get that same set of people to
> signoff on just temporarily disabling those pages on beta for this
> experiment. So seems everyone missed this!

Gijs points out I misread this -- 1274633 is only about removing the pages about private browsing and windows 10, not the sync-signup firstrun pages.

	Marco Bonardo [:mak]
	Comment 8 • 8 years ago
mozreview-review

Comment on attachment 8797188 [details]
Bug 1305770 - make cookies and passwords import from Chrome without locking,

https://reviewboard.mozilla.org/r/82780/#review82492

As usual, these things don't have good tests, so please ensure to test this and setup a QA session for Chrome migration.

::: browser/components/migration/ChromeProfileMigrator.js:111
(Diff revision 1)
>        errorAccumulator(e);
>      }
>    }
>  }
>  
> +function getRowsFromDBWithoutLocks(path, description, selectQuery) {

nit: I'm starting thinking you may want this in MigrationUtils...

	:Gijs (he/him) Assignee
	Comment 10 • 8 years ago
mozreview-review-reply

Comment on attachment 8797188 [details]
Bug 1305770 - make cookies and passwords import from Chrome without locking,

https://reviewboard.mozilla.org/r/82780/#review82492

> nit: I'm starting thinking you may want this in MigrationUtils...

Done!

	Pulsebot
	Comment 11 • 8 years ago

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d818379dc51c
make cookies and passwords import from Chrome without locking, r=mak

	Phil Ringnalda (:philor)
	Comment 12 • 8 years ago
bugherder

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

	:Gijs (he/him) Assignee
	Comment 13 • 8 years ago

Comment on attachment 8797188 [details]
Bug 1305770 - make cookies and passwords import from Chrome without locking,

Approval Request Comment
[Feature/regressing bug #]: allow importing cookies and passwords from Chrome to work even when Chrome is running
[User impact if declined]: when automatically importing data from Chrome, it's quite possible Chrome is still open, leading to a failure of the automatic migration. This patch addresses this.
[Describe test coverage new/current, TreeHerder]: there is automated test coverage for data import from Chrome. We don't have coverage for the case where Chrome is already running, but the basic assumption is that that can't get much worse (as the current state is utter failure).
[Risks and why]: low, it's still early on aurora, there's automated test coverage, for the manual import case we're keeping the warning telling users to close Chrome, and the current failure case can't really get any worse.
[String/UUID change made/needed]: nope

	Gerry Chang [:gchang]
	Comment 14 • 8 years ago

Comment on attachment 8797188 [details]
Bug 1305770 - make cookies and passwords import from Chrome without locking,

This patch fixes migrating data issue when chrome is still running which is a common behavior. I want to take this one in 51 aurora as early as possible.

Hi Florin,
Can we assign a QA to help verify this?

	Carsten Book [:Tomcat]
	Comment 15 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/f88c92402816

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 16 • 8 years ago

(In reply to Gerry Chang [:gchang] from comment #14)
> Hi Florin,
> Can we assign a QA to help verify this?

Added to the team's list for verification. Set "qe-verify+" flag for tracking.

Based on comment 13: "We don't have coverage for the case where Chrome is already running", so I think it would e useful to also do a bit more manual testing around auto-migration with Chrome running.

Gijs, is there anything else where you think it would be useful to get some additional manual testing? Note that we did NOT cover "Auto-migration" as a standalone feature (we did not know about it), so there is NO manual coverage here.

	:Gijs (he/him) Assignee
	Comment 17 • 8 years ago

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #16)
> Based on comment 13: "We don't have coverage for the case where Chrome is
> already running", so I think it would e useful to also do a bit more manual
> testing around auto-migration with Chrome running.
> 
> Gijs, is there anything else where you think it would be useful to get some
> additional manual testing? Note that we did NOT cover "Auto-migration" as a
> standalone feature (we did not know about it), so there is NO manual
> coverage here.

I think separate from automigration (where I'll ping in some other bugs, I suspect), this is the main issue that would actually affect the migration code also for the manual migration case, so besides this bug & bug 1285041 (which is basically the same issue as this bug but for chrome's history instead of passwords + cookies), I can't think of much. Thanks!

	Ciprian Georgiu, Desktop QA
	Comment 18 • 8 years ago

I've managed to reproduce this issue on 50.0b1 using STR form comment 0. 

I did some manual testing around auto-migration with Chrome running and I can confirm that all Bookmarks, History, Password and Cookies were imported (on Windows) successfully. Also, on Mac OS and Ubuntu I did not see any errors or problems with auto-migration, although passwords were not imported but this is expected as Gijs suggested in comment 1.

I think is safe to say that, this is verified fixed on latest Aurora 51.0a2 (2016-10-25) and latest Nightly 52.0a1 (2016-10-25) under the following OSes:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- Mac OS X 10.11.6

I will mark here accordingly. If you have any questions please let me know.