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

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

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

+++ 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.

Comment 1

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

Attachment: 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)

Comment 2

[Vladan Djeric (:vladan)]

I can reproduce this issue, please needinfo me when this lands and is ready for testing

Comment 3

[Richard Newman [:rnewman]]

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);

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e05387dd6b04

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

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

(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.

Comment 7

[Vladan Djeric (:vladan)]

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)

Comment 8

[Vladan Djeric (:vladan)]

I'll test the new build on my home machine tonight

Comment 9

[jorge alves]

(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.

Comment 10

[Vladan Djeric (:vladan)]

- 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

Comment 11

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

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 13

[Richard Newman [:rnewman]]

… but bear in mind that there is no places.sqlite on Android. browser.db is the closest thing.

Comment 14

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

(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.

Comment 15

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

(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

Comment 17

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

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.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 19

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

(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.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f456c40a9d7e

Comment 23

[Majo]

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!

Comment 24

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

(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

[Majo]

Thank you so very much!!!