1330384 - History tiles don't get removed right away when undoing the automigration

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Verdi [:verdi] Best to slack me]

Testing 51Beta 13 on Windows 10.

When the browser starts up, my data is successfully imported from Chrome and I see tiles on the new tab page that include results from my bookmarks and history. If I click the don't keep button, my imported bookmarks, history and passwords are instantly deleted but the tiles on the new tab page don't update to reflect that until after I restart the browser (opening another new tab is not enough).

Testing note: When using "clear recent history" with the new tab page open, the tiles for cleared history and bookmarked items immediately disappeared.

Comment 1

[Justin Dolske [:Dolske]]

I'd worry that unless this is a trivial matter of hooking up a Places history observer, this has probably missed the boat for the 51 funnelcake.

(Also, if we can do that, it should probably be conditional on the undo offer being shown, so that newtab pages are not forever listening to history changes. Sounds like a real (and likely existing) bug to fix, but just to minimize risk this late...)

Comment 2

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #1)
> I'd worry that unless this is a trivial matter of hooking up a Places
> history observer, this has probably missed the boat for the 51 funnelcake.
> 
> (Also, if we can do that, it should probably be conditional on the undo
> offer being shown, so that newtab pages are not forever listening to history
> changes. Sounds like a real (and likely existing) bug to fix, but just to
> minimize risk this late...)

The confusing thing is that the new tab page is already listening to history changes, and so this should already work, but doesn't seem to. Conversely, Verdi said that the menus and library are clearing correctly. So I'm not sure why this is broken at all. Seemed worth at least investigating.

Comment 3

[:Gijs (he/him)]

OK, I've talked with Marco and poked at this a bit, and we suspect that this is a bug in the tiles code that gets hit if you remove items for which no visits remain. I can also reproduce like this (on vanilla nightly):

1. clean profile
2. visit e.g. http://areweprettyyet.com/ by typing the url in the url bar
3. do the same again in a new tab
4. now open a new tab. areweprettyyet.com will be the first tile.
5. open the history sidebar (ctrl-h on windows, cmd-shift-h on OSX)
6. right click an entry for areweprettyyet.com and select 'forget about this site'

Expected: the tile goes away
Actual: the tile stays.

Fixing this "properly" is going to require updating the places/history observer in the new tab code. I'll look at doing this provided I can get it to work reasonably within a relatively short space of time and it's not a huge patch, because as dolske says, it might be too late for such an invasive change.

My fallback plan would be adding something to the automigration code, after the undo is complete, to force a newtab update. This will be guaranteed to only touch the funnelcake so should be safe for uplift either way.

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1330384 - force a refresh of the new tab page after automigration undo,

Review commit: https://reviewboard.mozilla.org/r/104230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104230/

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8826234 [details]
Bug 1330384 - force a refresh of the new tab page after automigration undo,

https://reviewboard.mozilla.org/r/104230/#review104962

::: browser/components/migration/AutoMigrate.jsm:218
(Diff revision 1)
>      yield this._removeUnchangedLogins(stateData.get("logins"));
>      histogram.add(25);
>  
> +    // This is async, but no need to wait for it.
> +    NewTabUtils.links.populateCache(() => {
> +      NewTabUtils.allPages.update(null, "newtab-should-update-for-real");

Looking at the implementation, it looks like this second argument isn't necessary, and you can just call NewTabUtils.allPages.update(null).

Better yet, NewTabUtils.allPages.update() should work too it looks.

I don't want to use "newtab-should-update-for-real" because I can see how in the future this will get cargo-culted around the codebase for no reason and we will regret it :)

Comment 6

[:Gijs (he/him)]

Comment on attachment 8826234 [details]
Bug 1330384 - force a refresh of the new tab page after automigration undo,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104230/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba7b38a8844d
force a refresh of the new tab page after automigration undo, r=jaws

Comment 8

[:Gijs (he/him)]

Comment on attachment 8826234 [details]
Bug 1330384 - force a refresh of the new tab page after automigration undo,

Approval Request Comment
[Feature/Bug causing the regression]: funnelcake for new migration experience
[User impact if declined]: users who decide they don't want the data we've imported for them might click the "Don't keep" button in the notification bar we show them while they're looking at the new tab page, and after clicking it they will get no feedback at all on that new tab page that we actually got rid of the data we imported (which is RIGHT THERE on the new tab page). That's pretty bad.
[Is this code covered by automated tests?]: the general code is covered by automated tests, as is the new tab code, but they clearly don't cover this case.
[Has the fix been verified in Nightly?]: not yet, but I manually repeatedly tested this in a VM, so I'm pretty confident it works.
[Needs manual test from QE? If yes, steps to reproduce]: Yes:
0. run a build containing this fix with a distribution folder containing the distribution.ini file attached to bug 1326252 on a machine where some other browser (IE, Edge, Chrome or Safari) is the default browser, and there are no Firefox profiles (empty the profiles folder)
1. in the window that opens, wait a bit (maybe 5-10 seconds) and then open about:newtab
2. click the "Don't keep" button in the notification bar that appears.

Expected:
any tiles that show up that relate to history from the browser that we imported from should disappear/rearrange to match effectively a "blank slate"

Actual (without this patch):
tiles that relate to history from the browser that we imported from remain.

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: because it's a change strictly to the automigration JSM code, which only runs in the funnelcake. It won't affect 51 as released.
[String changes made/needed]: no

Comment 9

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

Comment on attachment 8826234 [details]
Bug 1330384 - force a refresh of the new tab page after automigration undo,

This hasn't quite landed on m-c yet but I'm going to go ahead and approve it for uplift so we can test it before the beta build. If it ends up with obvious bad test failures on m-c please don't uplift, or back it out from all branches. Thanks!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5be69f2ff016
https://hg.mozilla.org/releases/mozilla-beta/rev/1fd589b5962b

Comment 11

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

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

Comment 12

[Andrei Vaida [:avaida]]

Flagging this for verification, str available in Comment 8.

Comment 13

[Marco Bonardo [:mak]]

Please remember to file a bug for the proper fix (that should likely involve adding onDeleteVisits the the PlacesProvider's history observer).

Comment 14

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #13)
> Please remember to file a bug for the proper fix (that should likely involve
> adding onDeleteVisits the the PlacesProvider's history observer).

Filed bug 1330934.

Comment 15

[Justin [:JW_SoftvisionQA]]

I am reopening this issue based on my findings on:  	

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

see video below:

https://jwilliams-softvision.tinytake.com/sf/MTI3MzY4M180Nzc2OTEy

As you can see it takes about 1 minute for my tabs, browser history, etc. to be automigrated over from Chrome, which freezes my browser. Once it is automigrated, Opening about:newtab does produce the dont keep option. On Win 7 and 10 the dont keep button works as intended but on win 8.1 it only deletes the bookmarks. The browser history and history tiles stay. Opening about:newtab again produces the same prompt "We automatically imported your data from Google Chrome. Would you like to keep it?". This prompt shows over and over on win 8.1.

Comment 16

[:Gijs (he/him)]

(In reply to Justin [:JW_SoftvisionQA] from comment #15)
> I am reopening this issue based on my findings on:  	
> 
> Mozilla/5.0 (Windows NT 6.3; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
> 
> see video below:
> 
> https://jwilliams-softvision.tinytake.com/sf/MTI3MzY4M180Nzc2OTEy
> 
> As you can see it takes about 1 minute for my tabs, browser history, etc. to
> be automigrated over from Chrome, which freezes my browser. Once it is
> automigrated, Opening about:newtab does produce the dont keep option. On Win
> 7 and 10 the dont keep button works as intended but on win 8.1 it only
> deletes the bookmarks. The browser history and history tiles stay. Opening
> about:newtab again produces the same prompt "We automatically imported your
> data from Google Chrome. Would you like to keep it?". This prompt shows over
> and over on win 8.1.

Hey Justin, thanks for reporting this. Can you stick it in a new bug report and needinfo me there? The issue Verdi reported in this bug was that while the history was correctly deleted (so the "most visited" and other menus would be empty) the new tab page itself didn't update. So we fixed that. It seems like on your Windows 8 machine something else is going on. For some reason we get stuck deleting the imported history and/or passwords. We'll need to fix that differently (it shouldn't get stuck, history should be removed everywhere, and we should probably also avoid showing the notification bar a second time while we're deleting stuff).

In the new bug, can you also include if, after you click "Don't Keep", do any errors appear in the browser console (and if so what errors)?

Comment 17

[Justin [:JW_SoftvisionQA]]

will do. thanks

Comment 18

[nagarajnaidu921]

[bugday-20170125] bug verified 
os: Ubuntu 16.04 LTS
I reproduced the bug and its fixed