Closed Bug 1330384 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: General, defect)

51 Branch
Unspecified
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox51 --- verified
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: verdi, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
Flags: needinfo?(gijskruitbosch+bugs)
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...)
(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.
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
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 :)
Attachment #8826234 - Flags: review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ba7b38a8844d
force a refresh of the new tab page after automigration undo, r=jaws
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
Attachment #8826234 - Flags: approval-mozilla-beta?
Attachment #8826234 - Flags: approval-mozilla-aurora?
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!
Attachment #8826234 - Flags: approval-mozilla-beta?
Attachment #8826234 - Flags: approval-mozilla-beta+
Attachment #8826234 - Flags: approval-mozilla-aurora?
Attachment #8826234 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/ba7b38a8844d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Flagging this for verification, str available in Comment 8.
Flags: qe-verify+
Flags: needinfo?(gwimberly)
Please remember to file a bug for the proper fix (that should likely involve adding onDeleteVisits the the PlacesProvider's history observer).
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1330934
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gwimberly)
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
(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)?
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jwilliams)
Resolution: --- → FIXED
will do. thanks
Flags: needinfo?(jwilliams)
[bugday-20170125] bug verified 
os: Ubuntu 16.04 LTS
I reproduced the bug and its fixed
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.