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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 months ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta9+)

Details

(Whiteboard: [fixed-in-places])

Attachments

(3 attachments, 5 obsolete attachments)

Assignee

Description

9 years ago
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.
Assignee

Updated

9 years ago
blocking2.0: --- → beta9+
Assignee

Comment 1

9 years ago
I'm not sure if I need to change tests here.  The current tests pass on the Places branch with this change.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Attachment #495884 - Flags: review?(philipp)
Assignee

Comment 2

9 years ago
Attachment #496219 - Flags: review?(philipp)
Assignee

Updated

9 years ago
Whiteboard: [needs review philiKON]
Attachment #496219 - Attachment is patch: true
Attachment #496219 - Attachment mime type: application/octet-stream → text/plain
Assignee

Comment 3

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

Let's have a reviewer race!
Attachment #495884 - Flags: review?(mconnor)
Assignee

Updated

9 years ago
Attachment #496219 - Flags: review?(mconnor)
Attachment #495884 - Flags: review?(philipp)
Attachment #495884 - Flags: review?(mconnor)
Attachment #495884 - Flags: review+
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.
Attachment #496219 - Flags: review?(philipp)
Attachment #496219 - Flags: review?(mconnor)
Attachment #496219 - Flags: review+
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.
Whiteboard: [needs review philiKON]
Assignee

Comment 6

9 years ago
(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.
Assignee

Comment 7

9 years ago
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.
Assignee

Comment 8

9 years ago
(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.
Assignee

Comment 9

9 years ago
Updated.  This should work much better.

We don't have any tests for 3.6, do we?
Attachment #495884 - Attachment is obsolete: true
Attachment #497562 - Flags: review?(philipp)
Assignee

Comment 10

9 years ago
Attachment #496219 - Attachment is obsolete: true
Attachment #497563 - Flags: review?(philipp)
Assignee

Comment 11

9 years ago
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.
Assignee

Updated

9 years ago
Attachment #497562 - Attachment is patch: true
Attachment #497562 - Attachment mime type: application/octet-stream → text/plain
Assignee

Comment 12

9 years ago
Updated per comments in person.
Attachment #497562 - Attachment is obsolete: true
Attachment #497567 - Flags: review?(philipp)
Attachment #497562 - Flags: review?(philipp)
Assignee

Comment 13

9 years ago
more comments!
Attachment #497567 - Attachment is obsolete: true
Attachment #497575 - Flags: review?(philipp)
Attachment #497567 - Flags: review?(philipp)
Assignee

Comment 14

9 years ago
Attachment #497563 - Attachment is obsolete: true
Attachment #497582 - Flags: review?(philipp)
Attachment #497563 - Flags: review?(philipp)
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)
Attachment #497575 - Flags: review?(philipp) → review+
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)
Attachment #497582 - Flags: review?(philipp) → review+
Preemptively landed on places branch to get test coverage on tinderboxes (a=sdwilsh): https://hg.mozilla.org/projects/places/rev/ef51960b24dd
Assignee

Updated

9 years ago
Whiteboard: [fixed-in-places]
Attachment #497653 - Flags: review?(mconnor) → review+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Remerged to m-c
https://hg.mozilla.org/mozilla-central/rev/9254211b7633

Merged follow-up:
https://hg.mozilla.org/mozilla-central/rev/14303443b88c
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 22

9 years ago
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)
blocking2.0: beta9+ → betaN+
Assignee

Comment 23

9 years ago
(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).
blocking2.0: betaN+ → beta9+
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.