Closed Bug 682027 Opened 8 years ago Closed 8 years ago

Sync needs an id for add-ons in the extensions database

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: gps, Assigned: gps)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [inbound])

Attachments

(2 files, 7 obsolete files)

To support add-on sync (bug 534956) the extensions database will need to be outfitted with a GUID field.

Currently, the extensions database contains an ID field. Unfortunately, Sync cannot reuse this field because the values are often known (defined by addons.m.o, etc) and Sync's privacy policy dictates that the synchronized IDs must not be deterministic. So, the solution is to add a new, locally-generated random GUID that Sync can use. The GUID will be managed by the Sync engine and will likely be of little interest to other clients, including the add-ons manager itself.

It is worth noting that the approach of adding a GUID to synchronized data sets to support Sync has already been performed on the history and bookmarks backends. See bug 607117 and related bugs.

I am attaching a work-in-progress patch. It is missing tests and isn't polished, but it should be enough for an informal review to catch fundamental issues before I progress too far down down the wrong avenue.
We already have an internal unique ID in the add-ons database for all installed add-ons. It is only an integer but I'm not sure what requirements you guys have beyond uniqueness here. Is that not enough?
We need a value that we can make unique across a Sync account. For that reason, the integer IDs that we already have in places, bookmarks, addons, etc. aren't practical to use. Imagine a user has addons A, B, C installed on one computer and addons B, C, D on her other. We need to reconcile the GUIDs for add-ons B, C on one of the machines (whichever syncs last) so that we end up with the set of A, B, C, D across all machines.
To elaborate, imagine on computer 1 we have the following ID to add-on mapping:

1: A
2: B
3: C

and on computer 2 we have

1: B
2: C
3: D

Let's say computer 1 syncs first and then computer 2. Upon download, it will recognize B and C as dupes and rewrite its IDs, leaving C and D with conflicting IDs. If computer 2 synced first and then computer 1, we'd have A with conflicting IDs. So unless we added code that went in and checked the whole database for dupes after each sync, using account-wide GUIDs is way easier.
I wonder if it makes sense to replace the existing numeric IDs with these GUIDs then
(In reply to Dave Townsend (:Mossop) from comment #4)
> I wonder if it makes sense to replace the existing numeric IDs with these
> GUIDs then

I think we should supplement instead of replace for the following reasons:

* We'd be changing the primitive type of "numeric_id" from integer to text.
* "numeric_id" is the primary key. Primary keys typically don't change, ever.
* Who knows who else (possibly add-ons) are storing the value of the ID and not expecting it to change. We would break these.

In a nutshell, supplementing is much safer than replacing.
I cleaned up the work-in-progress patch a bit and added some basic tests. Assigning to mossop for review. Not sure he is the right person, but I'm sure he knows who is.

General comments:

* The schema version is bumped. There is no getting around this since we're adding a new field.
* GUIDs are created just-in-time on row insertion. All rows should have GUIDs if they are in the database.
* The code for GUID creation was pretty much copied from services/sync. I feel dirty for violating DRY. If someone has a better solution, I'll implement it.
* The change to the 'getVisibleAddons' SQL query should set off alarm bells. If you look at getVisibleAddons() around line XPIProvider.jsm:4949, you'll see that the returned set of fields was different before this patch. Furthermore, I couldn't find any usages of the branch I was taking. I think the old version was accidentally inconsistent and I corrected it with my patch.
* I didn't reformat a number of lines containing SQL fields to maximize line usage. I thought this would clutter the patch.
* The unit tests are very basic. Ideally, I would test getVisibleAddons() directly, but I could find no existing direct test of it. I bowed to existing convention and opted to indirectly test.

The xpcshell tests for toolkit/mozapps/extensions all pass on my dev machine:
INFO | Result summary:
INFO | Passed: 200
INFO | Failed: 0
INFO | Todo: 0
Attachment #555788 - Attachment is obsolete: true
Attachment #555802 - Flags: review?(dtownsend)
Comment on attachment 555802 [details] [diff] [review]
add GUID field to extensions database

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

We should call this field syncID or something to make it clear what it is and what it is for, we already have way too much confusion over ID/GUID in the add-ons manager. I think you should add a constraint to the table to ensure the new id is always unique. Needs tests for setting the id, also verify that an add-on detected on startup gets an id.

I can't do a fuller review until I know the following:

Is it expected that upgrading to a new version of the same add-on gets a new id?
Do we care about maintaining the same id when the application upgrades and has to rebuild the extensions db?

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +4001,5 @@
>      getVisibleAddonForID: "SELECT " + FIELDS_ADDON + " FROM addon WHERE " +
>                            "visible=1 AND id=:id",
>      getVisibleAddoForInternalName: "SELECT " + FIELDS_ADDON + " FROM addon " +
>                                     "WHERE visible=1 AND internalName=:internalName",
> +    getVisibleAddons: "SELECT * FROM addon WHERE visible=1",

Please don't do this, correct the other case.

@@ +5067,5 @@
> +                        .join("");
> +      // Base64 encode
> +      aAddon.guid = btoa(byte_string).replace('+', '-', 'g')
> +                                     .replace('/', '_', 'g');
> +    }

Is there something wrong with just using nsIUUIDGenerator?
Attachment #555802 - Flags: review?(dtownsend) → review-
(In reply to Dave Townsend (:Mossop) from comment #7)
> We should call this field syncID or something to make it clear what it is
> and what it is for...

That's fair. Although, I raised this with philikon and he insisted on 'guid' rather than 'sync_id' (or similar). I'll let him state his point.

> I think you should add a constraint to the table to
> ensure the new id is always unique.

Will do.

> Needs tests for setting the id, also
> verify that an add-on detected on startup gets an id.

Will do.

> Is it expected that upgrading to a new version of the same add-on gets a new
> id?

Ideally it would maintain the same sync id. However, it isn't strictly required because Sync will reconcile the ids automagically. That being said, we should maintain the id if we can because it saves some technically unnecessary work.

> Do we care about maintaining the same id when the application upgrades and
> has to rebuild the extensions db?

Same answer as above.

> > +    getVisibleAddons: "SELECT * FROM addon WHERE visible=1",
> Please don't do this, correct the other case.

I would normally agree with you on principle of avoiding 'select *'. However, the reason I did it this way is because it was consistent with the other branch in the if-else in XPIProvider.getVisibleAddons(). The other branch is the only half that appeared in use (non-exhaustive audit) and I didn't want to break backwards-compatibility by taking away a few fields that others (including add-ons) might be relying on.

Can you please formally state you wish me to break backwards compatibility and limit both branches to a subset of fields?

> Is there something wrong with just using nsIUUIDGenerator?

Maybe.

UUIDs are a little longer than is needed, resulting in a slight overhead. But, I'm not too worried about that.

Second, I believe there is precedent in Sync land for using IDs of the coded format. Although, I'm not sure if that is a hard or fast rule. I'll defer to someone with more experience.

Finally, and most importantly, there could be privacy implications of using UUIDs, especially version 1 UUIDs, which store the MAC address and the time it was generated. Using a version 1 UUID, a party with access to the set of UUIDs could establish when and on what physical device an action was taken. This could be used to establish the physical whereabouts of an accused in court, etc. The privacy people have stated previously that they don't want the Sync IDs to leak information. I'm not sure if this would exceed their threshold, but I can find out. Of course, these concerns only apply to some UUID versions. Unfortunately, the documentation for nsIUUIDGenerator doesn't say what UUID version is returned, so I don't think we can rely on it being consistent (both now and in the future). If it had a guarantee that it would return version 4 UUIDs (the random ones), my privacy concerns would be muted.
(In reply to Gregory Szorc [:gps] from comment #8)
> > Is it expected that upgrading to a new version of the same add-on gets a new
> > id?
> 
> Ideally it would maintain the same sync id. However, it isn't strictly
> required because Sync will reconcile the ids automagically. That being said,
> we should maintain the id if we can because it saves some technically
> unnecessary work.

So I leave this choice up to you. I think it would be a useful thing to do, since perhaps others might rely on this ID for things, but doing it would of course complicate the patch and mean you'd have to write more tests ;)

Should you choose to do it then this function is the main place you'd do it: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#5138
There are possibly a couple of other points but I won't spend the time looking for those unless I know you definitely want to do this.

> > Do we care about maintaining the same id when the application upgrades and
> > has to rebuild the extensions db?
> 
> Same answer as above.

And basically the same response.

> 
> > > +    getVisibleAddons: "SELECT * FROM addon WHERE visible=1",
> > Please don't do this, correct the other case.
> 
> I would normally agree with you on principle of avoiding 'select *'.
> However, the reason I did it this way is because it was consistent with the
> other branch in the if-else in XPIProvider.getVisibleAddons(). The other
> branch is the only half that appeared in use (non-exhaustive audit) and I
> didn't want to break backwards-compatibility by taking away a few fields
> that others (including add-ons) might be relying on.

The other half should definitely be in use, at the very least in our xpcshell tests.

> Can you please formally state you wish me to break backwards compatibility
> and limit both branches to a subset of fields?

Yes please, I have previously been told by the sqlite experts that selecting * is problematic and best avoided so I changed all the statements to used defined fields then, the other branch is the one that got missed. A visual inspection confirms that FIELDS_ADDON contains everything in the schema and I'm pretty sure that we'd see test failures if we tried to access a field that wasn't in the results.
(In reply to Gregory Szorc [:gps] from comment #8)
> (In reply to Dave Townsend (:Mossop) from comment #7)
> > We should call this field syncID or something to make it clear what it is
> > and what it is for...
> 
> That's fair. Although, I raised this with philikon and he insisted on 'guid'
> rather than 'sync_id' (or similar). I'll let him state his point.

sync_guid or sync GUID is fine by me. I want to avoid 'id' or 'uuid' because these have different meanings.

> > Is it expected that upgrading to a new version of the same add-on gets a new
> > id?
> 
> Ideally it would maintain the same sync id.

Yes, absolutely. Take bookmarks, for instance. The bookmark itself keeps its GUID, even if its page title is updated.

> > Is there something wrong with just using nsIUUIDGenerator?
> 
> Maybe.
> 
> UUIDs are a little longer than is needed, resulting in a slight overhead.
> But, I'm not too worried about that.

We're trying very hard to standardize Sync GUIDs as 12 characters base64url. Most other engines are there yet or very close. I wouldn't want add-ons to break this again.
(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> (In reply to Gregory Szorc [:gps] from comment #8)
> > (In reply to Dave Townsend (:Mossop) from comment #7)
> > > We should call this field syncID or something to make it clear what it is
> > > and what it is for...
> > 
> > That's fair. Although, I raised this with philikon and he insisted on 'guid'
> > rather than 'sync_id' (or similar). I'll let him state his point.
> 
> sync_guid or sync GUID is fine by me. I want to avoid 'id' or 'uuid' because
> these have different meanings.

Unfortunately GUID also has different meanings, particularly in this context. What in particular are you concerned about with syncID?

> > > Is it expected that upgrading to a new version of the same add-on gets a new
> > > id?
> > 
> > Ideally it would maintain the same sync id.
> 
> Yes, absolutely. Take bookmarks, for instance. The bookmark itself keeps its
> GUID, even if its page title is updated.
> 
> > > Is there something wrong with just using nsIUUIDGenerator?
> > 
> > Maybe.
> > 
> > UUIDs are a little longer than is needed, resulting in a slight overhead.
> > But, I'm not too worried about that.
> 
> We're trying very hard to standardize Sync GUIDs as 12 characters base64url.
> Most other engines are there yet or very close. I wouldn't want add-ons to
> break this again.

Then they really aren't what most of the world calls "GUIDs" so I would much rather not call them that.
(In reply to Dave Townsend (:Mossop) from comment #11)
> Unfortunately GUID also has different meanings, particularly in this
> context. What in particular are you concerned about with syncID?

Sync already has confusing uses of ID and GUID and we're trying to standardize on GUID, that's all. I don't feel super strongly about this, but syncGUID seems like an ok compromise to me...

> > > UUIDs are a little longer than is needed, resulting in a slight overhead.
> > > But, I'm not too worried about that.
> > 
> > We're trying very hard to standardize Sync GUIDs as 12 characters base64url.
> > Most other engines are there yet or very close. I wouldn't want add-ons to
> > break this again.
> 
> Then they really aren't what most of the world calls "GUIDs" so I would much
> rather not call them that.

That baby fell in the bathwater years ago :(. We're just trying to standardize them to *something* and GUID was the more predominant term in the codebase, so it stuck. I feel like consistency is more important here than absolute correctness.

(Note that we distinguish GUID and UUID for a reason.)
(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> > What in particular are you concerned about with syncID?
> 
> Sync already has confusing uses of ID and GUID and we're trying to
> standardize on GUID, that's all. I don't feel super strongly about this, but
> syncGUID seems like an ok compromise to me...

Indeed, the exact term "syncID" has a specific meaning in Sync, beyond merely the other uses of "ID" and "GUID". If we must include the fragment "sync", then syncGUID would be my moderately strong preference.
This is a pre-patch to make getVisibleAddons() consistent in the fields that it fetches, per comment from mossop. It is backwards incompatible, but the existing xpcshell tests all pass.

Assuming this goes through, does the backwards incompatibility need to be documented anywhere?
Attachment #556100 - Flags: review?(dtownsend)
Attachment #556100 - Flags: review?(dtownsend) → review+
(In reply to Gregory Szorc [:gps] from comment #14)
> Assuming this goes through, does the backwards incompatibility need to be
> documented anywhere?

What are you suggesting is actually incompatible about this change?
(In reply to Dave Townsend (:Mossop) from comment #15)
> (In reply to Gregory Szorc [:gps] from comment #14)
> > Assuming this goes through, does the backwards incompatibility need to be
> > documented anywhere?
> 
> What are you suggesting is actually incompatible about this change?

Assuming there are columns not in FIELDS_ADDON, it will select less data than before. Maybe the set of columns is identical - I haven't verified.

Also, do you mind if I land this in services-central?
(In reply to Gregory Szorc [:gps] from comment #16)
> (In reply to Dave Townsend (:Mossop) from comment #15)
> > (In reply to Gregory Szorc [:gps] from comment #14)
> > > Assuming this goes through, does the backwards incompatibility need to be
> > > documented anywhere?
> > 
> > What are you suggesting is actually incompatible about this change?
> 
> Assuming there are columns not in FIELDS_ADDON, it will select less data
> than before. Maybe the set of columns is identical - I haven't verified.

I have and it looks identical to me.

> Also, do you mind if I land this in services-central?

Feel free
philikon just informed me I should have waited to land the patch until after all the parts in this bug are ready. Now I know and I won't do it again.

He also said that because the patch is so trivial, he doesn't think I need to back it out.
> > > Is it expected that upgrading to a new version of the same add-on gets a new
> > > id?
> > 
> > Ideally it would maintain the same sync id.
> 
> Yes, absolutely. Take bookmarks, for instance. The bookmark itself keeps its
> GUID, even if its page title is updated.
> 

After looking at the existing xpcshell tests (which seem to be a good source of the update scenarios), it seems this isn't as cut and dry as I was hoping.

Specifically, there are a lot of update cases I hadn't considered, such as add-ons changing their ID's during update. How we define the semantics of syncGUID in these cases will impact the behavior of Sync drastically.

For example, let's say we preserve the syncGUID between add-on ID changes. This means Sync will need to recognize the change in IDs. This could be difficult to implement on the Sync receiver, as we'd need to mirror the behavior of the add-on manager w.r.t. update APIs as closely is possible. I'm not sure how complicated or prone to future breakage due to add-on manager changes this would be.

On the other hand, if we say that syncGUID is tied to ID, updating through an ID change would result in downstream Sync clients having distinct uninstall and install records/events (as opposed to an "update" record). Would this be acceptable? Would this skew metrics at all?

I like the idea of defining the semantic meaning of syncGUID to be "Sync's version of the ID, reconciled across clients." This seems like the simplest to implement and most true to what it actually is. However, if it has metrics implications, then I suppose we need to introduce more complexity to Sync to solve the problem.

Anyone have any thoughts?
(In reply to Gregory Szorc [:gps] from comment #20)
> > > > Is it expected that upgrading to a new version of the same add-on gets a new
> > > > id?
> > > 
> > > Ideally it would maintain the same sync id.
> > 
> > Yes, absolutely. Take bookmarks, for instance. The bookmark itself keeps its
> > GUID, even if its page title is updated.
> > 
> 
> After looking at the existing xpcshell tests (which seem to be a good source
> of the update scenarios), it seems this isn't as cut and dry as I was hoping.
> 
> Specifically, there are a lot of update cases I hadn't considered, such as
> add-ons changing their ID's during update. How we define the semantics of
> syncGUID in these cases will impact the behavior of Sync drastically.
> 
> For example, let's say we preserve the syncGUID between add-on ID changes.
> This means Sync will need to recognize the change in IDs. This could be
> difficult to implement on the Sync receiver, as we'd need to mirror the
> behavior of the add-on manager w.r.t. update APIs as closely is possible.
> I'm not sure how complicated or prone to future breakage due to add-on
> manager changes this would be.
> 
> On the other hand, if we say that syncGUID is tied to ID, updating through
> an ID change would result in downstream Sync clients having distinct
> uninstall and install records/events (as opposed to an "update" record).
> Would this be acceptable? Would this skew metrics at all?

The ID change case is something new that very few add-ons probably use (certainly none on AMO). I think it is fine for that to appear as an uninstall/install in general however if something goes wrong and a sync client ends up with both add-ons or neither add-ons installed then that could be a problem.

> I like the idea of defining the semantic meaning of syncGUID to be "Sync's
> version of the ID, reconciled across clients." This seems like the simplest
> to implement and most true to what it actually is. However, if it has
> metrics implications, then I suppose we need to introduce more complexity to
> Sync to solve the problem.

I don't think the naming or any aspect of the sync ID has any relevance to metrics that I'm aware of.
Here is my revised patch to add a Sync GUID to the extensions database.

* 'guid' renamed to 'syncGUID' per feedback
* more unit tests
* add UNIQUE constraint on syncGUID column
* copy syncGUID on upgrade, but only if ID is the same

I'm not asking for a review yet because of the following open issues:

* need more test coverage
* need feedback (see below)

Areas for feedback:

* UNIQUE constraint. Do I check for duplicates before insertion, catch insertion failure in transaction, assume the GUID will be unique and don't worry about it, or other?
* Does it mostly look sane?
* Did I miss any feedback?
Attachment #555802 - Attachment is obsolete: true
Attachment #556187 - Flags: feedback?(dtownsend)
I've added a few more unit tests to this patch. Requested feedback from previous comments can be incorporated into the review.
Attachment #556187 - Attachment is obsolete: true
Attachment #556187 - Flags: feedback?(dtownsend)
Attachment #556697 - Flags: review?(dtownsend)
Summary: Add GUID field to extensions database → Sync needs an id for add-ons in the extensions database
If this field should be "unique for each add-on" (comment #2 and #3) and "should ideally remain the same when an add-on is upgraded" (comment #8) why not use the add-on ID (y'know, the thing which looks either like an email addy or like {xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx} where each x is a hex digit)? Since anything that Sync sends up into the cloud is encrypted on the client first, I don't see the privacy problem alluded to in comment #0.
(In reply to Tony Mechelynck [:tonymec] from comment #24)
> Since anything that Sync sends up into the cloud is encrypted on the
> client first, I don't see the privacy problem alluded to in comment #0.

Quite apart from any other reasons which Greg will explain, I will address this point:

Sync uses the ID of each record (which is what we're discussing here) as the 'public' ID for the record when transmitted and stored on the server. This ID isn't encrypted, it appears in logs, and it's more accessible to server intrusion or MITM attacks, etc.

We tend to be very leery of including identifiable information in Sync GUIDs. This is also why I'm not simply hashing the favicon URL for favicon sync: it becomes possible to identify which domains a user has visited.
This is the same as the previous patch but with bit rot removed. I've verified that all xpcshell and browser-chrome tests pass (on my Linux dev environment).
Attachment #556697 - Attachment is obsolete: true
Attachment #556697 - Flags: review?(dtownsend)
Attachment #571495 - Flags: review?(dtownsend)
Comment on attachment 571495 [details] [diff] [review]
Part 1: add syncGUID field to extensions database, v2

My kind of luck, this patch has already suffered from bit rot. Will upload a new version shortly.
Attachment #571495 - Flags: review?(dtownsend)
Fixed bit rot (again).
Attachment #571495 - Attachment is obsolete: true
Attachment #571842 - Flags: review?(dtownsend)
Comment on attachment 571842 [details] [diff] [review]
Part 1: add syncGUID field to extensions database, v3

Cancelling review for 2 reasons:

1) Patch will suffer bit rot from work to land in a few days
2) Patch needs to provide AddonManager.getAddonBySyncGUID API
Attachment #571842 - Flags: review?(dtownsend)
Blocks: 702819
Here is the latest version of the patch with changes referenced a few comments ago.

The patch is in Git format. I'll do the right thing for commit, or I can submit a Mercurial patch on request.

It does fail mochitests, but I don't think it is related to my changes. Maybe I need to perform a clobber after latest pull?

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_badargs.js | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: uncaught exception: Error: Incorrect arguments passed to InstallTrigger.install() at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_badargs2.js | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: uncaught exception: Error: Missing URL property for 'Unsigned XPI' at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_cancel.js | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: self.runningInstalls is null at chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/head.js:138
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_enabled3.js | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: callback is not a function at chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/head.js:157
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_localfile2.js | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: uncaught exception: Error: insufficient permissions to install: [xpconnect wrapped nsIURI @ 0x10ad981d0 (native @ 0x108f47038)] at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/browser_whitelist3.js | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: self.runningInstalls is null at chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/xpinstall/head.js:138
Attachment #571842 - Attachment is obsolete: true
Attachment #574750 - Flags: review?(dtownsend)
(In reply to Gregory Szorc [:gps] from comment #31)
> Created attachment 574750 [details] [diff] [review]
> Part 1: add syncGUID field to extensions database, v4
> 
> Here is the latest version of the patch with changes referenced a few
> comments ago.
> 
> The patch is in Git format. I'll do the right thing for commit, or I can
> submit a Mercurial patch on request.
> 
> It does fail mochitests, but I don't think it is related to my changes.
> Maybe I need to perform a clobber after latest pull?

Can you push to try and see if the errors show up there too?
Attachment #574750 - Attachment is patch: true
Comment on attachment 574750 [details] [diff] [review]
Part 1: add syncGUID field to extensions database, v4

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

Could you add a test to show that trying to set a duplicate syncGUID throws and doesn't make it to the database.

At the moment this patch throws away syncGUIDs on DB upgrade and there really isn't a straightforward way for sync to detect that and correct it. I'd suggest adding it to the list of things migrated. I was planning on writing some nice code to make that migration simpler but probably quickest to just do the same as the rest does for now.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +5262,5 @@
>    addAddonMetadata: function XPIDB_addAddonMetadata(aAddon, aDescriptor) {
> +    // Create a GUID if one does not exist
> +    if (!aAddon.syncGUID) {
> +      let rng = Cc["@mozilla.org/security/random-generator;1"]
> +                  .createInstance(Ci.nsIRandomGenerator);

Normal style used for such statements is:

let rng = Cc["@mozilla.org/security/random-generator;1"].
          createInstance(Ci.nsIRandomGenerator);

@@ +5268,5 @@
> +      let byte_string = [String.fromCharCode(byte) for each (byte in bytes)]
> +                        .join("");
> +      // Base64 encode
> +      aAddon.syncGUID = btoa(byte_string).replace('+', '-', 'g')
> +                                         .replace('/', '_', 'g');

If we accidentally generate a duplicate syncGUID here then adding the add-on will fail and it will disappear for the user. Talk to me about how unlikely that is.

@@ +5385,5 @@
> +      // The Sync GUID gets carried over only if the add-on is the same.
> +      // By forcefully setting the new GUID to null, we ensure a new one
> +      // is generated.
> +      aNewAddon.syncGUID = aOldAddon.id == aNewAddon.id ? aOldAddon.syncGUID
> +                                                        : null;

This will only happen for the same ID so this additional test isn't necessary.
(In reply to Dave Townsend (:Mossop) from comment #34)
> @@ +5268,5 @@
> > +      let byte_string = [String.fromCharCode(byte) for each (byte in bytes)]
> > +                        .join("");
> > +      // Base64 encode
> > +      aAddon.syncGUID = btoa(byte_string).replace('+', '-', 'g')
> > +                                         .replace('/', '_', 'g');
> 
> If we accidentally generate a duplicate syncGUID here then adding the add-on
> will fail and it will disappear for the user. Talk to me about how unlikely
> that is.

The GUID is a random 64-bit number. So, the probability is # of installed extensions divided by 2^64. In other words, practically guaranteed uniqueness. I'm assuming the random numbers have equivalent probability of occurring.
Try run for df2e47277475 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=df2e47277475
Results (out of 63 total builds):
    success: 57
    warnings: 6
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-df2e47277475
(In reply to Gregory Szorc [:gps] from comment #35)
> (In reply to Dave Townsend (:Mossop) from comment #34)
> > @@ +5268,5 @@
> > > +      let byte_string = [String.fromCharCode(byte) for each (byte in bytes)]
> > > +                        .join("");
> > > +      // Base64 encode
> > > +      aAddon.syncGUID = btoa(byte_string).replace('+', '-', 'g')
> > > +                                         .replace('/', '_', 'g');
> > 
> > If we accidentally generate a duplicate syncGUID here then adding the add-on
> > will fail and it will disappear for the user. Talk to me about how unlikely
> > that is.
> 
> The GUID is a random 64-bit number. So, the probability is # of installed
> extensions divided by 2^64. In other words, practically guaranteed
> uniqueness. I'm assuming the random numbers have equivalent probability of
> occurring.

Sounds good enough
I believe I have addressed all review comments with this patch.
Attachment #574750 - Attachment is obsolete: true
Attachment #574750 - Flags: review?(dtownsend)
Attachment #575254 - Flags: review?(dtownsend)
Attachment #575254 - Attachment is patch: true
Comment on attachment 575254 [details] [diff] [review]
add syncGUID field to addons, v5

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

::: toolkit/mozapps/extensions/test/xpcshell/test_syncGUID.js
@@ +84,5 @@
> +        restartManager();
> +
> +        AddonManager.getAddonsByIDs(installIDs, function(addons) {
> +          try {
> +            addons[1].syncGUID = addons[0].syncGUID;

Add a do_throw after this line so the test doesn't hang if for some reason it doesn't throw.

@@ +89,5 @@
> +          }
> +          catch (e) {
> +            do_check_eq(e.result,
> +                        Components.results.NS_ERROR_STORAGE_CONSTRAINT);
> +            run_next_test();

restart the manager and get the add-on and verify its syncGUID is still what you expect here.
Attachment #575254 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/b97604b1c038
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Added .syncGUID to https://developer.mozilla.org/en/Addons/Add-on_Manager/Addon
Added .getAddonBySyncGUID() to https://developer.mozilla.org/en/Addons/Add-on_Manager/AddonManager
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla11 → ---
Huh?
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Blocks: 704973
You need to log in before you can comment on or make changes to this bug.