664590 - History sync: ignore file:/// URIs

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Philipp von Weitershausen [:philikon]]

file:/// URIs are very likely not going to make sense on other computers. They only clutter up awesomebar results.

Comment 1

[Richard Newman [:rnewman]]

Something to consider as we move towards some kind of "profile in the cloud", or more durable single-device 'backup' solutions — we have to think about somehow storing data that only a single client cares about. We don't want to sync them, but …

Comment 2

[Edouard Oger [:eoger]]

Bumping this so we see it in our weekly triage.
I think this is a valuable bug to fix, we can either filter the file:// uris in the tracker [0] or before syncing [1]

[0] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/services/sync/modules/engines/history.js#380,400
[1] https://searchfox.org/mozilla-central/rev/a5d613086ab4d0578510aabe8653e58dc8d7e3e2/services/sync/modules/engines/history.js#78

Comment 3

[Edouard Oger [:eoger]]

Attachment: Bug 664590 - Do not track file:/// visits.

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8935139 [details]
Bug 664590 - Do not track file:/// visits.

https://reviewboard.mozilla.org/r/205980/#review211626

I think we probably need a similar change in getAllIDs (which probably means using a constant or function to work out if a URL qualifies). We might also want to consider not applying incoming records that meet the criteria (ie, over time, this change should end up meaning no file:// URLs end up on the server, but we might as well stop the ones that are already there being applied locally).

Comment 5

[Richard Newman [:rnewman]]

N.B., for devices that are using Sync as a backup, this is data loss — you sync, you restore, and you are missing bits of history.

I would think very carefully about whether it's better to have URIs that you've visited but can't use on this particular device, or whether it's better to add this complexity so that some history dies with a particular profile.

It's also worth thinking: if file: URIs aren't backed up to the cloud, should they be saved in history at all?

FWIW, I've never heard a user complain about this; Ed's Comment 2 implies that he sees value in the change, but I wanted to capture that there are tradeoffs here.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Thanks Richard, I agree there are tradeoffs and my opinions are weakly held.

(In reply to Richard Newman [:rnewman] from comment #5)
> N.B., for devices that are using Sync as a backup, this is data loss — you
> sync, you restore, and you are missing bits of history.

Yeah, but we try to discourage users seeing this as a backup because it's quite possible it will fail to actually do this for them if they are a single device user.

OTOH, from the POV of a multi-device Sync user, this is introducing garbage on their other profiles.

> I would think very carefully about whether it's better to have URIs that
> you've visited but can't use on this particular device, or whether it's
> better to add this complexity so that some history dies with a particular
> profile.

I think it all hinges on the word "backup".

> It's also worth thinking: if file: URIs aren't backed up to the cloud,
> should they be saved in history at all?

Again, the word "backup" :) If we ever had something designed to backup a profile to the cloud, then yes, we should also include those visits. But Sync is not that.

> FWIW, I've never heard a user complain about this; Ed's Comment 2 implies
> that he sees value in the change, but I wanted to capture that there are
> tradeoffs here.

By the same token, I can't really imagine users complaining if they did manage to successfully use Sync as a backup service and got all history back other than file:// URIs.

FWIW, my history has many file:// URLs pointing at Sync logs and so are even more tightly bound to a profile rather than a device.

I have personally seen this behaviour (eg, a file URI pointing at, roughly %TEMP%/test.html be synced to my other devices) and just thought "heh - silly sync" rather than filed a bug - so I don't feel strongly either way. But I don't think the case for syncing them is stronger than the case for not syncing them.

There's also moz-extension URLs that we probably want to blacklist in the same way - there's no bug on file for history, but bug 1412083 asks for us not to sync prefs that have them.

Comment 7

[Edouard Oger [:eoger]]

Comment on attachment 8935139 [details]
Bug 664590 - Do not track file:/// visits.

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

Comment 8

[Edouard Oger [:eoger]]

Thanks Mark, I amended my patch.
I also think that if we insist so much on telling users that Sync is not a backup solution, we might as well ignore these local visits.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8935139 [details]
Bug 664590 - Do not track file:/// visits.

https://reviewboard.mozilla.org/r/205980/#review214042

Looks great, thanks!

Comment 10

[Edouard Oger [:eoger]]

Comment on attachment 8935139 [details]
Bug 664590 - Do not track file:/// visits.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d57aa14966b9
Do not track file:/// visits. r=markh

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

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