1555173 - An invalid synced bookmark might be replaced with an equally invalid local copy

Status: RESOLVED DUPLICATE of bug 1615931

(Firefox :: Sync, enhancement, P3)

Description

[Lina Butler [:lina]]

The Sync mirror has three validity states for synced bookmarks:

• Valid means the remote item is valid, and can be applied as-is.
• Reupload means the remote item is valid, but we fixed up its URL—we can still apply it, but should also flag it for reupload after merging. We use this to rewrite legacy tag queries.
• Replace means the remote item isn't valid at all (a bookmark with a missing or invalid URL), and there's no way we can apply it. We either replace it with a local copy if one exists, or delete it on the server.

The problem is, we assume all local items are Valid, which isn't always true. A local tree might have a bookmark with an invalid URL (bug 1554936, bug 1553377, bug 1554202), and reuploading it won't make a difference—we'll just churn uploading the same invalid bookmark on every sync.

I think we'll want to validate URLs when we build the local tree, and flag any with invalid URLs, so that our merger can delete them instead of getting into a loop reuploading a bookmark that (eventually) no device can support.

Deleting the bookmark outright is extreme, but...I'm really not sure what else we can do. We can't apply it, because storing it is going to create all kinds of problems (bug 1401401, bug 458619). Maybe we can add it as a query param to about:neterror, rewriting the URL to something like about:neterror?e=malformedURI&uri=<the percent-encoded original invalid URL>, so that we at least preserve it? Or export the invalid URLs into a separate file? All of that is extra complexity, but might save folks from wondering why Sync suddenly removed a bookmark that they'd been using.

Comment 1

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

(In reply to Lina Cambridge (she/her) [:lina] from comment #0)

Maybe we can add it as a query param to about:neterror, rewriting the URL to something like about:neterror?e=malformedURI&uri=<the percent-encoded original invalid URL>, so that we at least preserve it?

If the added complexity isn't too large, this sounds like a really good compromise and keeps enough context so a motivated user has enough of the original URL that they can manually take further action if they really care.

Comment 2

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #1)

If the added complexity isn't too large, this sounds like a really good compromise and keeps enough context so a motivated user has enough of the original URL that they can manually take further action if they really care.

Yep. We might also want to hide these rewritten about:neterror URLs in the UI on iOS, or we'll run into the same thing as we did for about:reader...but we already want to hide place: queries on iOS and Fenix, anyway, so we may be able to piggyback on that.

Comment 3

[Lina Butler [:lina]]

I rolled this up into bug 1615931, and went with the more aggressive approach of deleting invalid bookmarks.