616001 - Sync needs to check moz_places.guid and moz_bookmarks.guid if it exists

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Shawn Wilsher :sdwilsh]

Bug 607117 adds a a guid column to moz_places and moz_bookmarks that Sync should check for before it checks the annotation table.

This blocks the Places branch merge, and should land on Places (either through a merge of your hg repo to Places, or by itself) prior to us merging Places into mozilla-central.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - update the history engine v1.0

I'm not sure if I need to change tests here.  The current tests pass on the Places branch with this change.

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - udpate the bookmark engine v1.0

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 495884 [details] [diff] [review]
Part 1 - update the history engine v1.0

Let's have a reviewer race!

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 496219 [details] [diff] [review]
Part 2 - udpate the bookmark engine v1.0

>+  get _haveTempTablesStm() {
>+    return this._getStmt(
>+      "SELECT name FROM sqlite_temp_master " +
>+      "WHERE name IN ('moz_places_temp', 'moz_historyvisits_temp')");
>+  },
>+
>+  get _haveTempTables() {
>+    if (this.__haveTempTables == null)
>+      this.__haveTempTables = !!Utils.queryAsync(this._haveTempTablesStm,
>+                                                 ["name"]).length;
>+    return this.__haveTempTables;
>+  },

Why introduce this here and not use the try+catch approach (which you already used in the history part)?

>   get _guidForIdStm() {
>-    let stmt = this._getStmt(
>-      "SELECT a.content AS guid " +
>-      "FROM moz_items_annos a " +
>-      "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
>-      "JOIN moz_bookmarks b ON b.id = a.item_id " +
>-      "WHERE n.name = :anno_name AND b.id = :item_id");
>-    stmt.params.anno_name = GUID_ANNO;
>-    return stmt;
>+    if (this._haveTempTables) {
>+      // Gecko <2.0
>+      let stmt = this._getStmt(
>+        "SELECT a.content AS guid " +
>+        "FROM moz_items_annos a " +
>+        "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
>+        "JOIN moz_bookmarks b ON b.id = a.item_id " +
>+        "WHERE n.name = :anno_name AND b.id = :item_id");
>+      stmt.params.anno_name = GUID_ANNO;
>+      return stmt;
>+    }

In history you UNION results from the guid column and the guid annotation (in case the guid column exists). Shouldn't we do that here as well? I think so because Sync 1.6 and 4.0b8, for instance, will always write to the annotation. People might time-travel to these with their profile, so there's no guarantee that the existence of the guid column means the GUID is located there. Right?

>@@ -947,13 +983,22 @@ BookmarksStore.prototype = {
>   },
> 
>   get _idForGUIDStm() {
>-    let stmt = this._getStmt(
>-      "SELECT a.item_id AS item_id " +
>-      "FROM moz_items_annos a " +
>-      "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
>-      "WHERE n.name = :anno_name AND a.content = :guid");
>-    stmt.params.anno_name = GUID_ANNO;
>-    return stmt;
>+    if (this._haveTempTables) {
>+      // Gecko <2.0
>+      let stmt = this._getStmt(
>+        "SELECT a.item_id AS item_id " +
>+        "FROM moz_items_annos a " +
>+        "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
>+        "WHERE n.name = :anno_name AND a.content = :guid");
>+      stmt.params.anno_name = GUID_ANNO;
>+      return stmt;
>+    }

Same concern as above.

>+    // Gecko 2.0
>+    return this._getStmt(
>+      "SELECT id AS item_id " +
>+      "FROM moz_bookmarks " +
>+      "WHERE guid = :guid");
>   },

Looks good otherwise, r=me with the concern addressed.

Comment 5

[Philipp von Weitershausen [:philikon]]

This should land on fx-sync, btw. We'll catch it in our next m-c merge. If you guys need a merge to places earlier, I can do that for you too.

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Why introduce this here and not use the try+catch approach (which you already
> used in the history part)?
It's needed for the setGuid case at least.

> In history you UNION results from the guid column and the guid annotation (in
> case the guid column exists). Shouldn't we do that here as well? I think so
> because Sync 1.6 and 4.0b8, for instance, will always write to the annotation.
> People might time-travel to these with their profile, so there's no guarantee
> that the existence of the guid column means the GUID is located there. Right?
Actually, it won't always with this patch.  If we don't have the temporary tables, we just write the guid to the actual table.  However, we do need to do a UNION here.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 495884 [details] [diff] [review]
Part 1 - update the history engine v1.0

>+++ b/services/sync/modules/engines/history.js
>+  __guidStmt: null,
>   get _guidStm() {
>+    if (this.__guidStmt) {
>+      return this.__guidStmt;
>+    }
>+
>+    let moz_placesQuery =
>+      "SELECT guid " +
>+      "FROM moz_places " +
>+      "WHERE url = :page_url";
>+
>+    let stmt;
>     if (this._haveTempTables) {
>       // Gecko <2.0
>+      let annotationQuery =
>+        "SELECT a.content AS guid " +
>+        "FROM moz_annos a " +
>+        "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
>         "JOIN ( " +
>           "SELECT id FROM moz_places_temp WHERE url = :page_url " +
>           "UNION " +
>           "SELECT id FROM moz_places WHERE url = :page_url " +
>         ") AS h ON h.id = a.place_id " +
>+        "WHERE n.name = :anno_name";
>+      // Try to first read from moz_places.  Creating the statement will throw
>+      // if the column doesn't exist, though so fallback to just reading from
>+      // the annotation table.
>+      try {
>+        stmt = this._getStmt(moz_placesQuery + " UNION " + annotationQuery);
>+      }
>+      catch (e) {
>+        stmt = this._getStmt(annotationQuery);
>+      }
>+      stm.params.anno_name = GUID_ANNO;
Interestingly enough, this code is wrong because we won't bind the GUID_ANNO as needed when we reuse the statement.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> (In reply to comment #4)
> > Why introduce this here and not use the try+catch approach (which you already
> > used in the history part)?
> It's needed for the setGuid case at least.
And...the setGuid code in both patches is also wrong.  Ugh.

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - update the history engine v1.1

Updated.  This should work much better.

We don't have any tests for 3.6, do we?

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - udpate the bookmark engine v1.1

Comment 11

[Shawn Wilsher :sdwilsh]

The only thing we need to write a test for here is for 3.6 using a gecko 2.0 database that is downgraded.  We want to make sure that Sync writes the guid to moz_bookmarks and moz_places and not to the annotation tables.  We also need to check that it reads from there, and not the annotation tables.

Comment 12

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - update the history engine v1.2

Updated per comments in person.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: Part 1 - update the history engine v1.3

more comments!

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: Part 2 - udpate the bookmark engine v1.2

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 497575 [details] [diff] [review]
Part 1 - update the history engine v1.3

r=me with GUIDForUri fix as discussed in person (I will fix before landing)

Comment 16

[Philipp von Weitershausen [:philikon]]

Comment on attachment 497582 [details] [diff] [review]
Part 2 - udpate the bookmark engine v1.2

r=me with GUIDForId fix as discussed in person (I will fix before landing)

Comment 17

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 - tests for v11 places db in Firefox 3.5/3.6

Comment 18

[Philipp von Weitershausen [:philikon]]

Preemptively landed on places branch to get test coverage on tinderboxes (a=sdwilsh): https://hg.mozilla.org/projects/places/rev/ef51960b24dd

Comment 19

[Philipp von Weitershausen [:philikon]]

Landed on fx-sync:

https://hg.mozilla.org/services/fx-sync/rev/2fc58c7e6193
https://hg.mozilla.org/services/fx-sync/rev/473e8aad8209
https://hg.mozilla.org/services/fx-sync/rev/9617adf089f0

Merged to mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/193be5701a2c

Comment 20

[Philipp von Weitershausen [:philikon]]

Had to back it out on m-c because of test failures:
https://hg.mozilla.org/mozilla-central/rev/9ea3e2d8b202

sdwilsh and I landed a fix already in fx-sync:
https://hg.mozilla.org/services/fx-sync/rev/2585ad3e9fa9

Fix also merged to places branch:
https://hg.mozilla.org/projects/places/rev/705ec6a77ab0

Leaving this open until we reland on mozilla-central.

Comment 21

[Philipp von Weitershausen [:philikon]]

Remerged to m-c
https://hg.mozilla.org/mozilla-central/rev/9254211b7633

Merged follow-up:
https://hg.mozilla.org/mozilla-central/rev/14303443b88c

Comment 22

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> As per today's meeting, beta 9 will be a time-based release. Marking these all
> betaN+. Please move it back to beta9+ if  you believe it MUST be in the next
> beta (ie: trunk is in an unshippable state without this)
This would need to block beta 9 if we had to back it out (unlikely).