add-on syncGUID being set twice

RESOLVED FIXED in Firefox 11

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Assignee)

Description

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 http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#7277
(Assignee)

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
Status: NEW → ASSIGNED
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(install.name, "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("bootstrap1@tests.mozilla.org", function(b1) {
>        do_check_neq(b1, null);
>        do_check_eq(b1.version, "1.0");
> +      do_check_neq(b1.syncGUID);

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

Comment 4

6 years ago
Pushed to inbound with extra null argument to do_check_eq() added: https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d2a543a0d1

Comment 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/f8d2a543a0d1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

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+
(Assignee)

Comment 8

6 years ago
Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/e03d758c3c08
(Assignee)

Updated

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

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.

Updated

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