1188170 - Log raw URI string when PlacesUtils.bookmarks.getBookmarkURI() failed with NS_ERROR_MALFORMED_URI

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

In bug 1182366 we added special handling for invalid bookmarks. We should attempt to log the string from the database row to get some handle on what kinds of invalid bookmarks we are seeing

Comment 1

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

Attachment: 0001-Bug-1188170-log-the-url-string-when-the-Sync-bookmar.patch

Does some raw SQL to get the URL string when a nsURI can't be constructed. Even has a test (although the test doesn't exercise the exception handler - I'm not sure how to provoke an exception)

Comment 2

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

Comment on attachment 8641427 [details] [diff] [review]
0001-Bug-1188170-log-the-url-string-when-the-Sync-bookmar.patch

Mak, your feedback would be appreciated if you can find a few minutes

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8641427 [details] [diff] [review]
0001-Bug-1188170-log-the-url-string-when-the-Sync-bookmar.patch

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

LGTM!

::: services/sync/modules/engines/bookmarks.js
@@ +282,5 @@
> +              // using a cached statement - we should rarely hit this.).
> +              let url;
> +              try {
> +                let stmt = this._store._getStmt(`
> +                      SELECT p.url

This is my first multiline string in JS. Mind. Blown.

@@ +290,5 @@
> +                stmt.params.id = id;
> +                let rows = Async.querySpinningly(stmt, ["url"]);
> +                url = rows.length == 0 ? "<not found>" : rows[0].url;
> +              } catch (ex) {
> +                let desc = ex.toString();

Put this in an `else` just below; save doing it unnecessarily.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8641427 [details] [diff] [review]
0001-Bug-1188170-log-the-url-string-when-the-Sync-bookmar.patch

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

Can't wait the day Sync will use PlacesUtils.promiseDBConnection that is a Sqlite.jsm wrapped read-only connection :)

::: services/sync/modules/engines/bookmarks.js
@@ +276,5 @@
>              try {
>                uri = PlacesUtils.bookmarks.getBookmarkURI(id);
>              } catch (ex) {
>                // Bug 1182366 - NS_ERROR_MALFORMED_URI here stops bookmarks sync.
> +              // Try and get the string value of the URL for diagnostic purposes

nit: would be nice to move this to a separate helper function, for readability

@@ +283,5 @@
> +              let url;
> +              try {
> +                let stmt = this._store._getStmt(`
> +                      SELECT p.url
> +                      FROM moz_places p

nit: for convention in Places we use h alias for moz_places (yes I know it sounds "wrong" but p is used for other things)

@@ +291,5 @@
> +                let rows = Async.querySpinningly(stmt, ["url"]);
> +                url = rows.length == 0 ? "<not found>" : rows[0].url;
> +              } catch (ex) {
> +                let desc = ex.toString();
> +                // poor-man's |instanceof mozIStorageError| check.

instanceof doesn't work?

@@ +293,5 @@
> +              } catch (ex) {
> +                let desc = ex.toString();
> +                // poor-man's |instanceof mozIStorageError| check.
> +                if (ex && ex.message && ex.result) {
> +                  desc = "Storage error: " + ex.message + " (" + ex.result + ")";

a template string would be nicer

@@ +297,5 @@
> +                  desc = "Storage error: " + ex.message + " (" + ex.result + ")";
> +                }
> +                url = "<failed: " + desc + ">";
> +              }
> +              this._log.warn("Deleting bookmark with invalid URI. url=\"${url}\", id=${id}",

shouldn't this be a template string for ${} to work? (that would also remove the need to escape double apices)

::: services/sync/tests/unit/test_bookmark_invalid.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

The license header is no more needed in tests.

@@ +57,5 @@
> +add_task(function* test_ignore_invalid_uri() {
> +  _("Ensure that we don't die with invalid bookmarks.");
> +
> +  // First create a valid bookmark.
> +  let bmid = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,

nit: ideally in new code we should use the new promise based bookmarks API from Bookmarks.jsm...
but since Sync is very legacy I guess I should ignore this...

@@ +65,5 @@
> +
> +  // Now update moz_places with an invalid url.
> +  let connection = PlacesUtils.history
> +                              .QueryInterface(Ci.nsPIPlacesDatabase)
> +                              .DBConnection;

please use PlacesUtils.withConnectionWrapper (it takes a description of the operation and a task, the task gets a Sqlite.jsm connection.
you can then easily .execute() a query, and yield everything.
We should stop using .DBConnection

@@ +94,5 @@
> +                                                  PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                                  "the title");
> +
> +  // Now update moz_bookmarks to reference a non-existing places ID
> +  let connection = PlacesUtils.history

ditto

Comment 5

[Marco Bonardo [:mak]]

I should probably clarify, for the sake of completeness, that PlacesUtils.withConnectionWrapper exposes a read/write connection while PlacesUtils.promiseDBConnection is a read-only connection. The reason for the read-only connection is that it's concurrent (doesn't wait for writes).

Comment 6

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

Thanks for the review/feedback!

> This is my first multiline string in JS. Mind. Blown.

:)

> Put this in an `else` just below; save doing it unnecessarily.

done.

(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #4)
> nit: would be nice to move this to a separate helper function, for
> readability

Done.

> nit: for convention in Places we use h alias for moz_places (yes I know it
> sounds "wrong" but p is used for other things)

:) - but done

> instanceof doesn't work?

It does - don't know what I was thinking!

> a template string would be nicer

Done - they can blow rnewman's mind again!

> > +              this._log.warn("Deleting bookmark with invalid URI. url=\"${url}\", id=${id}",
> 
> shouldn't this be a template string for ${} to work? (that would also remove
> the need to escape double apices)

FWIW, Log.jsm takes a regular string with "${}" inserts and an additional object to use for the substitutions. Log.jsm is even smarter though - if the object can be JSON.stringified() it is, and if it is an exception it formats it with a stack. (Log.jsm is dumber in lots of ways though - eg, it can't handle calling a function)

But in this specific case we don't rely on either of these niceties, so to be consistent with the other code I'm adding I changed this to a template string too.

> The license header is no more needed in tests.

Removed.

> nit: ideally in new code we should use the new promise based bookmarks API
> from Bookmarks.jsm...
> but since Sync is very legacy I guess I should ignore this...

I ignored this too ;)

> please use PlacesUtils.withConnectionWrapper

Done.

Thanks!

Comment 7

[Pulsebot]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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