Bookmark sync: NS_ERROR_MALFORMED_URI in PlacesUtils.bookmarks.getBookmarkURI()

RESOLVED FIXED

Status

Cloud Services
Firefox Sync: Backend
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

(Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

(Whiteboard: [fxsync])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
+++ This bug was initially created as a clone of Bug #622657 +++

Bug 622657 comment 29 has a log for Firefox 42 that shows getBookmarkURI() failing during a sync and thus killing the Sync for bookmarks. We should try and do the same as we did in that bug but a few lines down.
(Assignee)

Comment 1

2 years ago
Created attachment 8635888 [details] [diff] [review]
0001-Bug-1182366-avoid-an-invalid-bookmark-from-preventin.patch

I just saw this same error on https://www.reddit.com/r/firefox/comments/3dqu32/sync_encountered_an_error_while_syncing_unknown/. This patch takes the same approach we did in bug 622657 (ie, catch the error and safely attempt to delete the offending bookmark)
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8635888 - Flags: review?(rnewman)
I can reproduce this issue, please needinfo me when this lands and is ready for testing
Comment on attachment 8635888 [details] [diff] [review]
0001-Bug-1182366-avoid-an-invalid-bookmark-from-preventin.patch

Review of attachment 8635888 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/engines/bookmarks.js
@@ +270,5 @@
>            } catch(ex) {}
>  
>            if (queryId)
>              key = "q" + queryId;
> +          else {

Can we add braces for the `if` part, too?

@@ +276,5 @@
> +            try {
> +              uri = PlacesUtils.bookmarks.getBookmarkURI(id);
> +            } catch (ex) {
> +              // Bug 1182366 - NS_ERROR_MALFORMED_URI here stops bookmarks sync.
> +              this._log.warn("Deleting bookmark with invalid URI. id", id);

"…id: " + id);
Attachment #8635888 - Flags: review?(rnewman) → review+

Comment 4

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/e05387dd6b04
https://hg.mozilla.org/mozilla-central/rev/e05387dd6b04
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 6

2 years ago
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #2)
> I can reproduce this issue, please needinfo me when this lands and is ready
> for testing

Thanks! It would actually be awesome if you could take a copy of places.db before trying to reproduce - the fix will attempt to delete the bookmark with an invalid URI, so if it works as expected we've probably lost the ability to again reproduce or find the underlying bookmark causing the problem.
Flags: needinfo?(vdjeric)
I backed up my places.sqlite DBs on my 2 Windows desktops, but I don't know if there's a convenient way  to do it on my Android phone (not rooted)
Flags: needinfo?(vdjeric)
I'll test the new build on my home machine tonight
Flags: needinfo?(vdjeric)

Comment 9

2 years ago
(In reply to Mark Hammond [:markh] from comment #6)
> (snip) we've probably lost the
> ability to again reproduce or find the underlying bookmark causing the
> problem.

I fixed my issue by deleting the bookmark manually but, IIRC, it was a wikipedia url that was added automatically by an old version of firefox. I have no clue which one because my profile is old.
- The new Nightly build fixed my bookmarks syncing problem
- Can you log the offending URL to about:sync-log?
- I couldn't precisely find which bookmark was causing the problem, because I deleted a few bookmarks manually myself when I first ran into the problem. Also, I could not export the places.sqlite on my Android phone.

Anyway, I diffed the moz_bookmarks before & after updating my desktop Nightly and these are the suspects:

1. On one machine I changed the URL of a keyword search from https://phonebook.mozilla.org/search.php?format=html&query=%s to https://phonebook.mozilla.org/#search/%s
2. This bookmark did not exist after the update https://www.google.ca/search?q=define:%s but I may have deleted it manually :(
3. Ditto for "about:firefox" from mobile
Flags: needinfo?(vdjeric)
(Assignee)

Comment 11

2 years ago
Thanks very much for taking the time to verify this.

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #10)
> - The new Nightly build fixed my bookmarks syncing problem
> - Can you log the offending URL to about:sync-log?

Sadly there's no URL to log - the problem is that we get NS_ERROR_MALFORMED_URI fetching it. We do log an entry "this._log.warn("Deleting bookmark with invalid URI. id=XXXX" where XXXX is the primary key for the bookmark record in places.db.

> 1. On one machine I changed the URL of a keyword search from
> https://phonebook.mozilla.org/search.php?format=html&query=%s to
> https://phonebook.mozilla.org/#search/%s
> 2. This bookmark did not exist after the update
> https://www.google.ca/search?q=define:%s but I may have deleted it manually
> :(
> 3. Ditto for "about:firefox" from mobile

I've got keyword bookmarks and an about:* URL in my bookmarks, so I doubt that is it. 'https://www.google.ca/search?q=define:%s' looks like a candidate given the ':' in the middle, but things seem to work OK for me with that as a bookmark. I'm not sure how to identify it further.
I'm currently affected by this issue and I'm regularly run 2 machines with Developer Edition (though I switched from Nightly to dev edition maybe a week ago on one) and Nightly on Android. I haven't tried the latest Nightly update.

I have a list of my bookmarks (`select uri from moz_places, moz_bookmarks where moz_bookmarks.fk = moz_places.id`;) that I can send privately if it'd be helpful.

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #10)
> Also, I could not export the places.sqlite on my Android phone.

You can use copy profile: https://wiki.mozilla.org/Mobile/Fennec/Android/Development/Addons#Copy_Profile
… but bear in mind that there is no places.sqlite on Android. browser.db is the closest thing.
(Assignee)

Comment 14

2 years ago
(In reply to Michael Comella (:mcomella) from comment #12)
> I have a list of my bookmarks (`select uri from moz_places, moz_bookmarks
> where moz_bookmarks.fk = moz_places.id`;) that I can send privately if it'd
> be helpful.

It probably would, thanks! Note however that the patch to delete the invalid bookmark has already landed on Nightly, so hopefully you can grab it before that runs for you.
(Assignee)

Comment 15

2 years ago
(In reply to Mark Hammond [:markh] from comment #14)
> It probably would, thanks! Note however that the patch to delete the invalid
> bookmark has already landed on Nightly, so hopefully you can grab it before
> that runs for you.

Oops, I see you already sent it to rnewman, so there's no need to send it to me
(Assignee)

Updated

2 years ago
Depends on: 1188170

Updated

2 years ago
Iteration: --- → 42.2 - Jul 27
Whiteboard: [fxsync]
Duplicate of this bug: 1197637
(Assignee)

Comment 17

2 years ago
Comment on attachment 8635888 [details] [diff] [review]
0001-Bug-1182366-avoid-an-invalid-bookmark-from-preventin.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Sync fails to work.
[Describe test coverage new/current, TreeHerder]: Existing tests pass, been on 42 for some time now.
[Risks and why]: Low
[String/UUID change made/needed]: None

We are seeing a number of reports in the wild of this bug, so I think we should take it on beta so it hits release ASAP.
Attachment #8635888 - Flags: approval-mozilla-beta?
Mark, would you be able to confirm that this issue is gone from Nightly? You mentioned that there are a lot of reports that this is impacting our end-users. Do we have any verification/confirmation from any users that the issue is gone in Nightly? This will give me lots more confidence uplifting this to Beta.
Flags: needinfo?(markh)
(Assignee)

Comment 19

2 years ago
(In reply to Ritu Kothari (:ritu) from comment #18)
> Mark, would you be able to confirm that this issue is gone from Nightly? You
> mentioned that there are a lot of reports that this is impacting our
> end-users.

Unfortunately this is difficult to reproduce - it implies some minor corruption of the places database. However, the new test arranges for such corruption and works.

> Do we have any verification/confirmation from any users that the
> issue is gone in Nightly?

Comment 10 indicates this is so.

> This will give me lots more confidence uplifting this to Beta.

This is a very localized and small fix - I'm confident it will do no harm.
Flags: needinfo?(markh)
Comment on attachment 8635888 [details] [diff] [review]
0001-Bug-1182366-avoid-an-invalid-bookmark-from-preventin.patch

Based on Mark's comment that this is a low risk fix, let's uplift to Beta41. We want sync to work well in all scenarios.
Attachment #8635888 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/f456c40a9d7e
status-firefox41: --- → fixed
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1234236

Comment 23

2 years ago
I'm sorry, I am not that computer savvy and cannot understand how to fix the issue on my end. Can someone please translate this into non tech language?

Thanks!
(Assignee)

Comment 24

2 years ago
(In reply to Majo from comment #23)
> I'm sorry, I am not that computer savvy and cannot understand how to fix the
> issue on my end. Can someone please translate this into non tech language?

Sorry Majo! From bug 1234236, your Firefox is version 38.5 but the bug you have encountered was fixed in version 41. The most recent version is 42 - so visiting visiting https://www.mozilla.org/en-US/firefox/new/ might help you get this newer, fixed version. If this is not your PC, you need to talk to your network administrator to help you with this. Unfortunately, your problem is likely to remain until you update to a newer version.

Comment 25

2 years ago
Thank you so very much!!!
You need to log in before you can comment on or make changes to this bug.