add-on syncGUID being set twice

RESOLVED FIXED in Firefox 11



Add-ons Manager
6 years ago
6 years ago


(Reporter: gps, Assigned: gps)


Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed)


(Whiteboard: [qa-])


(2 attachments)



6 years ago
I noticed that it is possible for a syncGUID to get generated twice during some install scenarios.

I /think/ it reproduces for non-restartless add-ons only:

1) Install add-on from AddonInstall. In the onInstallEnded, the addon.syncGUID is defined as one thing.
2) Restart the application
3) The add-on actually gets installed and syncGUID is regenerated.

I would expect the syncGUID from the first run to get carried over.

This isn't a deal-breaker: Sync will reconcile the different syncGUIDs as things are synchronized. But, it is annoying.

I'll address this as a follow-up to add-on sync.
I think you just need to add syncGUID to here

Comment 2

6 years ago
Created attachment 583334 [details] [diff] [review]
Reuse syncGUID, v1

Adding the syncGUID field to importMetadata seemed to do it!

I verified the new test code in test_install.js failed without the change to XPIProvider.jsm applied. test_bootstrap.js worked both before and after (I think that makes sense).

My biggest concern with this patch is that I don't have enough test coverage. Is there anywhere else I really need test coverage?
Assignee: nobody → gps
Attachment #583334 - Flags: review?(dtownsend)
Comment on attachment 583334 [details] [diff] [review]
Reuse syncGUID, v1

Review of attachment 583334 [details] [diff] [review]:

::: toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js
@@ +148,5 @@
>      do_check_eq(install.type, "extension");
>      do_check_eq(install.version, "1.0");
>      do_check_eq(, "Test Bootstrap 1");
>      do_check_eq(install.state, AddonManager.STATE_DOWNLOADED);
> +    do_check_neq(install.addon.syncGUID);

do_check_neq expects two arguments, I guess this is just checking it isn't undefined but be explicit about it

@@ +189,5 @@
>      AddonManager.getAddonByID("", function(b1) {
>        do_check_neq(b1, null);
>        do_check_eq(b1.version, "1.0");
> +      do_check_neq(b1.syncGUID);

Attachment #583334 - Flags: review?(dtownsend) → review+

Comment 4

6 years ago
Pushed to inbound with extra null argument to do_check_eq() added:

Comment 5

6 years ago
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12

Comment 6

6 years ago
Created attachment 585054 [details] [diff] [review]
Reuse syncGUID, v2

This is the actual patch that was committed. Adding as attachment because I'm requesting approval for Aurora. I would like this merged to Aurora because it improves the add-on sync experience and reduces the potential for bugs due to bug 710448.
Attachment #585054 - Flags: review+
Attachment #585054 - Flags: approval-mozilla-aurora?

Comment 7

6 years ago
Comment on attachment 585054 [details] [diff] [review]
Reuse syncGUID, v2

[Triage Comment]
Because of where we are in the cycle, and the possibility for data loss as outlined in 710448, I'm approving for Aurora.
Attachment #585054 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 8

6 years ago
Pushed to aurora:


6 years ago
status-firefox11: --- → fixed
gps: is there anything to verify here?

Comment 10

6 years ago
Tracy: This was really an internal optimization. I'm not sure there is an easy way to verify this beyond verifying the Add-on Manager xpcshell tests pass.


6 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.