Sync doesn't upload any bookmarks when one bookmark is invalid
Categories
(Firefox :: Sync, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox75 | --- | fixed |
People
(Reporter: lebow.physics.8b, Assigned: lina)
References
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0
Steps to reproduce:
Sync (either Sync Now or wait for it to sync), when one bookmark was an invalid URL and there were other (valid) new bookmarks locally that needed to be uploaded.
The invalid bookmark was https://http%3a%2f%2fwww.forbes.com%2fsites%2fsarahtilton%2f2015%2f01%2f09%2fproblems-with-math-tutoring-app-expands-the-answers%2f/
The local device where I saw the error is a laptop whose OS is Xubuntu 18.04, and the Firefox is Mozilla Firefox for Ubuntu 73.0 (64 bit). I sync it with another partition of that same laptop's hard disk (same OS), and with a OnePlus One phone running Firefox for Android (OS is not stock Android, it's CyanogenOS).
Actual results:
New (valid) local bookmarks did not appear on my two other devices (so I infer they never got uploaded). On the device with the bad bookmark (the laptop), new bookmarks that had been created on another device appeared in Bookmarks (so I infer that downloading is fine). No error message (unless I examine the sync log). Bookmarks I had moved to new folders returned to the folders they had been in before.
Expected results:
The new valid local bookmarks should be uploaded to the server and then appear on other devices. Bookmarks locally moved to other folders should appear in the new folders on other devices.
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Thanks for reporting this! We definitely shouldn't be crashing when we try to fetch the frecency for the bookmark. I'm curious how that invalid URL made it into the database in the first place, though—was that Forbes bookmark one you added locally, or was it synced from another device in the past?
It's hard to remember now how the invalid URL got there, because it would have happened in December when I started having problems, but it must have been added locally. Also, I think I edited that bookmark manually (going to Library and clicking on the bookmark), to trim tracking junk off the end of the link, though I would not have manually added the extra "https://" at the beginning.
Comment 3•4 years ago
|
||
Lina self-assigned this, otherwise I would just have closed it as a dupe - but I'll let her sort out what she wants to do and where, so see-also it is!
Assignee | ||
Comment 4•4 years ago
•
|
||
So there are a couple things we can do here:
- For this specific bug, let's add a
try...catch
to_calculateIndex
here, so that fetching the frecency doesn't fail. This will still upload a record for the bookmark, and leave it to the other devices to either replace it with a locally valid copy, if they have a valid URL, or delete it if not. - In the general case of "what happens when a local bookmark has an invalid URL", that's bug 1555173. Bug 1470043 considered adding a maintenance task to periodically validate and clean up invalid URLs in Places, but that would amount to a full table scan on
moz_places
every time we ran maintenance, which is very expensive. We can validate URLs for local bookmarks when we build the tree—we're scanningmoz_bookmarks
, anyway, and that's much smaller thanmoz_places
—but the default delete-if-invalid behavior is kinda hostile, especially if the server doesn't have a valid copy. But, given that's what would happen now in a more roundabout way, and the fact that parts of our front-end don't deal well with invalid URLs, it might be the best approach.
Comment 5•4 years ago
|
||
(In reply to :Lina Cambridge from comment #4)
So there are a couple things we can do here:
- For this specific bug, let's add a
try...catch
to_calculateIndex
here, so that fetching the frecency doesn't fail. This will still upload a record for the bookmark, and leave it to the other devices to either replace it with a locally valid copy, if they have a valid URL, or delete it if not.
+1
- In the general case of "what happens when a local bookmark has an invalid URL", that's bug 1555173. Bug 1470043 considered adding a maintenance task to periodically validate and clean up invalid URLs in Places, but that would amount to a full table scan on
moz_places
every time we ran maintenance, which is very expensive.
How about a more intrusive maint task that isn't run by default - our existing scan could then trigger that task instead of delete-if-invalid?
(while I think I'm fine with delete-if-invalid in the general case, this specific bug bothers me, as it seems the invalid URL was added somewhat recently and I don't understand what paths we expose to actually do that. In the "old" days it was easy to blame rogue addons, but now?)
Assignee | ||
Comment 6•4 years ago
|
||
This commit changes Store::local_row_to_item
to validate local URLs,
and flags items with malformed URLs as invalid. These items are either
replaced with valid remote copies, if they exist, or deleted if not.
Additionally, BaseBookmarksStore#_calculateIndex
no longer throws
when determining the sort index for an item with an invalid URL.
As an aside, we use the url
crate to parse URLs. This is the same
crate as used by MozURL
, which, in turn, backs the JS URL
constructor...so URLs should be validated the same way in Rust and JS.
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 9•4 years ago
|
||
(In reply to Mark Hammond [:markh] [:mhammond] from comment #5)
(while I think I'm fine with delete-if-invalid in the general case, this specific bug bothers me, as it seems the invalid URL was added somewhat recently and I don't understand what paths we expose to actually do that. In the "old" days it was easy to blame rogue addons, but now?)
Yeah, this is concerning to me, too—AFAICT, everywhere we INSERT INTO
or UPDATE moz_places
, we're working with an nsIURI
or URL
object, so the URL must have been valid before. I wondered if nsIURIFixup
might be getting in the way, but that already requires a valid input URL. I think we should record telemetry for this, and emit it as part of these counts, so we can see how frequently we hit it.
Assignee | ||
Comment 10•4 years ago
|
||
I filed https://github.com/mozilla/dogear/issues/58 for plumbing the telemetry through from Dogear.
Comment 11•4 years ago
|
||
Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c47f5848279 Handle invalid bookmark URLs in new bookmark sync. r=markh,LougeniaBailey
Comment 12•4 years ago
|
||
bugherder |
Reporter | ||
Comment 13•4 years ago
|
||
Thank you for fixing the bug! I really appreciate it. (Yes, this is a late thank you, oh well.)
Description
•