710961 - add-on syncGUID being set twice

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Gregory Szorc [:gps]]

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.

Comment 1

[Dave Townsend [:mossop]]

I think you just need to add syncGUID to here http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#7277

Comment 2

[Gregory Szorc [:gps]]

Attachment: 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?

Comment 3

[Dave Townsend [:mossop]]

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("[e-mail]", function(b1) {
>        do_check_neq(b1, null);
>        do_check_eq(b1.version, "1.0");
> +      do_check_neq(b1.syncGUID);

ditto

Comment 4

[Gregory Szorc [:gps]]

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

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f8d2a543a0d1

Comment 6

[Gregory Szorc [:gps]]

Attachment: 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.

Comment 7

[Alex Keybl [:akeybl]]

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.

Comment 8

[Gregory Szorc [:gps]]

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/e03d758c3c08

Comment 9

[Tracy Walker [:tracy]]

gps: is there anything to verify here?

Comment 10

[Gregory Szorc [:gps]]

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.